Change profilers to use thread local evacuation counters #59741

davmason · 2021-09-29T08:55:55Z

Using one global evacuation counter per profiler was causing lots of contention in hot calls to CORProfilerPresent, this change switches to using one per thread.

It also switches hot CORProfiler* methods to FORCEINLINE, and changes CORProfilerPresent to be a much cheaper check, especially when a notification profiler is present.

ghost · 2021-09-29T08:56:04Z

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Using one global evacuation counter per profiler was causing lots of contention in hot calls to CORProfilerPresent, this change switches to using one per thread.

It also switches hot CORProfiler* methods to FORCEINLINE, and changes CORProfilerPresent to be a much cheaper check, especially when a notification profiler is present.

Author:	davmason
Assignees:	-
Labels:	`area-Diagnostics-coreclr`
Milestone:	-

src/coreclr/inc/profilepriv.inl

src/coreclr/vm/profilinghelper.cpp

davmason · 2021-09-29T19:27:17Z

Looks like there's also a CRST ordering issue: Assert failure(PID 9716 [0x000025f4], Thread: 12940 [0x328c]): Consistency check failed: Crst Level violation: Can't take level 12 lock CrstThreadStore because you already holding level 0 lock CrstProfilingAPIStatus, it should be fixed by adding the right relationship in crsttypes.def, I will update that later this afternoon.

jkotas · 2021-09-30T13:09:16Z

src/coreclr/vm/eetoprofinterfaceimpl.cpp

Does this mean that the profiler detach can crash if it happens to interleave with a callback issued where GetThread is null?

How many of the callbacks are affected? Did we have this problem before?

For all of desktop and before my change in coreclr the evacuation counters were on the Thread * and it would only increment/decrement if GetThread returned non-null. So this behavior should be identical to 5.0 and previous runtimes.

I have been thinking since last night about if there was a latent bug. Mostly I don't think so, but there is a couple places like EventPipe callbacks that may not have a managed thread

yeah, I am pretty suspicious there is a latent bug here when it comes to EventPipe notifications - they can be triggered on threads where GetThread() == NULL

@noahfalk thinking about it though, with the profiler they are synchronous on the thread that it happens. We may not actually have a bug, since I would expect any place EventPipe events happen to have an EE thread. Since we have the managed bridge from native events to EventPipe.

This issue also affects server GC threads. Server GC threads do not have Thread*.

I would expect any place EventPipe events happen to have an EE thread

I'm ~98% sure this isn't the case : ) Some of the EventPipe events are emitted where GetThread()==NULL (such as the events from Server GC threads @jkotas mentioned). Originally the code in EventPipe's WriteEvent() code refered to GetThread() for the thread-local write buffers but that was broken. We had to invent the new EventPipeThread object to work around the lack of Thread* for some events.

Ok, I agree there is a latent issue here, but it also has been present ever since the EventPipe stuff was merged in 3.1. Given that and that there's also an easy workaround - stop the session before detaching - I propose I file a bug and don't try to solve this now.

#60081

Yeah, I wasn't pushing to have you fix it now given that it was pre-existing, just confirming that it does exist.

jkotas

LGTM. Thank you!

src/coreclr/inc/crsttypes.h

src/coreclr/inc/profilepriv.inl

Change to prefix increment

jkotas · 2021-10-06T12:34:21Z

src/coreclr/inc/profilepriv.inl

 }

 inline HRESULT ProfControlBlock::JITInlining(FunctionID callerId, FunctionID calleeId, BOOL *pfShouldInline)
 {
    LIMITED_METHOD_CONTRACT;

+    *pfShouldInline = TRUE;


This will initialize *pfShouldInline deterministically for the first profiler only.

Should this line rather be in JITInliningHelper instead? I think you can even delete pfShouldInline argument for the JITInliningHelper and have BOOL fShouldInline local in JITInliningHelper instead.

src/coreclr/inc/profilepriv.h

jkotas · 2021-10-07T02:57:20Z

~~There is still unresolved feedback: #59741 (comment) . Do you plan to address it before merging?~~

I have not seen the last commit for some reason.

jkotas

Looks great!

jkotas · 2021-10-07T03:20:09Z

/backport to release/6.0

github-actions · 2021-10-07T03:20:22Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1314583486

github-actions · 2021-10-07T03:22:06Z

@jkotas backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Change profilers to use thread local evacuation counters
.git/rebase-apply/patch:191: trailing whitespace.
    return (&g_profControlBlock)->mainProfilerInfo.pProfInterface.Load() != NULL 
.git/rebase-apply/patch:1056: trailing whitespace.
        DWORD newValue = 
.git/rebase-apply/patch:1067: trailing whitespace.
        DWORD newValue = 
warning: 3 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	src/coreclr/inc/profilepriv.inl
M	src/coreclr/vm/profilinghelper.cpp
M	src/coreclr/vm/proftoeeinterfaceimpl.cpp
M	src/coreclr/vm/threads.cpp
M	src/coreclr/vm/threads.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/vm/threads.h
Auto-merging src/coreclr/vm/threads.cpp
Auto-merging src/coreclr/vm/proftoeeinterfaceimpl.cpp
Auto-merging src/coreclr/vm/profilinghelper.cpp
Auto-merging src/coreclr/inc/profilepriv.inl
Applying: get rid of lambdas
.git/rebase-apply/patch:153: trailing whitespace.
        
.git/rebase-apply/patch:166: trailing whitespace.
    return pProfilerInfo->pProfInterface->IsCallback3Supported(); 
.git/rebase-apply/patch:194: trailing whitespace.
    return AnyProfilerPassesCondition(&RequiresGenericsContextForEnterLeaveHelper); 
warning: 3 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	src/coreclr/inc/profilepriv.inl
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/inc/profilepriv.inl
CONFLICT (content): Merge conflict in src/coreclr/inc/profilepriv.inl
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 get rid of lambdas
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

* Change profilers to use thread local evacuation counters Change to prefix increment * get rid of lambdas * Fix jit inlining, fix R2R too * Remove VolatilePtr<> from helpers * Get rid of additionalData argument

…0116) * Change profilers to use thread local evacuation counters Change to prefix increment * get rid of lambdas * Fix jit inlining, fix R2R too * Remove VolatilePtr<> from helpers * Get rid of additionalData argument

davmason requested review from a team and jkotas September 29, 2021 08:55

ghost added the area-Diagnostics-coreclr label Sep 29, 2021

jkotas reviewed Sep 29, 2021

View reviewed changes

src/coreclr/inc/profilepriv.inl Outdated Show resolved Hide resolved

jkotas reviewed Sep 29, 2021

View reviewed changes

src/coreclr/inc/profilepriv.inl Outdated Show resolved Hide resolved

josalem reviewed Sep 29, 2021

View reviewed changes

src/coreclr/vm/profilinghelper.cpp Outdated Show resolved Hide resolved

jkotas reviewed Sep 30, 2021

View reviewed changes

davmason force-pushed the notification_profiler_perf branch from da6b788 to 080d379 Compare September 30, 2021 16:40

jkotas approved these changes Sep 30, 2021

View reviewed changes

am11 reviewed Oct 3, 2021

View reviewed changes

src/coreclr/inc/crsttypes.h Outdated Show resolved Hide resolved

jkotas reviewed Oct 5, 2021

View reviewed changes

src/coreclr/inc/profilepriv.inl Outdated Show resolved Hide resolved

jkotas reviewed Oct 5, 2021

View reviewed changes

src/coreclr/inc/profilepriv.inl Outdated Show resolved Hide resolved

davmason added 4 commits October 6, 2021 03:06

Change profilers to use thread local evacuation counters

1a0fea7

Change to prefix increment

get rid of lambdas

c30e4cc

Fix jit inlining, fix R2R too

c46a4df

Remove VolatilePtr<> from helpers

e8e39fd

davmason force-pushed the notification_profiler_perf branch from 2257ec5 to e8e39fd Compare October 6, 2021 10:09

Replace static contracts that I accidentally overwrote

cb927c3

jkotas reviewed Oct 6, 2021

View reviewed changes

src/coreclr/inc/profilepriv.h Outdated Show resolved Hide resolved

davmason added 2 commits October 6, 2021 13:03

Get rid of additionalData argument

126bd62

Initialize params for every call

e9b9cc2

jkotas approved these changes Oct 7, 2021

View reviewed changes

jkotas merged commit 240a64d into dotnet:main Oct 7, 2021

davmason mentioned this pull request Oct 10, 2021

Add test for profiler inlining #60225

Merged

ghost locked as resolved and limited conversation to collaborators Nov 6, 2021

Change profilers to use thread local evacuation counters #59741

Change profilers to use thread local evacuation counters #59741

Uh oh!

Conversation

davmason commented Sep 29, 2021

Uh oh!

ghost commented Sep 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davmason commented Sep 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkotas commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

jkotas commented Oct 7, 2021

Uh oh!

github-actions bot commented Oct 7, 2021

Uh oh!

github-actions bot commented Oct 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jkotas commented Oct 7, 2021 •

edited

Loading