Runtime: SampleProfiler incorrectly marks all samples as external code on Windows

[josalem]

The SampleProfiler sends ThreadSample events that reference stacks. The payload of these events is supposed to indicate what type of code was executing in the leaf frame _at the time of suspension_.

https://github.com/dotnet/runtime/blob/3f8966546a09ec104ca958eab20a22463907fed1/src/coreclr/src/vm/sampleprofiler.cpp#L238-L250

The SampleProfiler uses a cached value of m_fPreemptiveGCDisabled to determine whether to mark a sample as "managed" (GC was in COOP mode) or "external" (GC was in preemptive mode).

https://github.com/dotnet/runtime/blob/73a79932b9bfcf2078f21f173661f2d4afed9e90/src/coreclr/src/vm/threads.h#L4627-L4654

Currently, SaveGCModeOnSuspension is only called from HandleGCSuspensionForInterruptedThread.

https://github.com/dotnet/runtime/blob/c8c07900afdbf2103c1ccf9719005535edec851b/src/coreclr/src/vm/threadsuspend.cpp#L6071-L6096

Unfortunately, this code is only used for thread suspension on _non_-Windows platforms. This means that all sample events on Windows are marked as "external" code since the underlying value is never changed from the uninitialized value of 0 (meaning preemptive mode). On Linux/Mac systems, however, stacks are correctly labeled.

This value is used in tools like PerfView to do analysis on things like CPU blocking time.

We need to determine a location to call SaveGCModeOnSuspension that works for both platforms or add a call to the equivalent location on Windows.

One spot I'm vetting is here:
https://github.com/dotnet/runtime/blob/93cbc0974aa0475c586f40645e5b58a4b08ef017/src/coreclr/src/vm/threadsuspend.cpp#L3519-L3526

in ThreadSuspend::SuspendRuntime where it enumerates all the threads it is about to suspend.

I'm not sure how long this has been an issue, but I'm guessing it has been since this logic went in (~4 years ago circa .NET Core 2.2).

Is there another way we could transmit this information? We could save the 4 bytes _per_ sample event if we do away with this payload. That can add up fast when you have ~1000 samples _per_ thread _per_ second. That's roughly 4 kb/s per thread for a value that will _always_ be 1 or 2. Could we intern this information directly into the stack events? That would require a format change, though.

CC @sywhang @noahfalk @brianrob @tommcdon

[msftbot[bot]]

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

Issue Details

---

The SampleProfiler sends ThreadSample events that reference stacks. The payload of these events is supposed to indicate what type of code was executing in the leaf frame _at the time of suspension_.

https://github.com/dotnet/runtime/blob/3f8966546a09ec104ca958eab20a22463907fed1/src/coreclr/src/vm/sampleprofiler.cpp#L238-L250

The SampleProfiler uses a cached value of m_fPreemptiveGCDisabled to determine whether to mark a sample as "managed" (GC was in COOP mode) or "external" (GC was in preemptive mode).

https://github.com/dotnet/runtime/blob/73a79932b9bfcf2078f21f173661f2d4afed9e90/src/coreclr/src/vm/threads.h#L4627-L4654

Currently, SaveGCModeOnSuspension is only called from HandleGCSuspensionForInterruptedThread.

https://github.com/dotnet/runtime/blob/c8c07900afdbf2103c1ccf9719005535edec851b/src/coreclr/src/vm/threadsuspend.cpp#L6071-L6096

Unfortunately, this code is only used for thread suspension on _non_-Windows platforms. This means that all sample events on Windows are marked as "external" code since the underlying value is never changed from the uninitialized value of 0 (meaning preemptive mode). On Linux/Mac systems, however, stacks are correctly labeled.

This value is used in tools like PerfView to do analysis on things like CPU blocking time.

We need to determine a location to call SaveGCModeOnSuspension that works for both platforms or add a call to the equivalent location on Windows.

One spot I'm vetting is here:
https://github.com/dotnet/runtime/blob/93cbc0974aa0475c586f40645e5b58a4b08ef017/src/coreclr/src/vm/threadsuspend.cpp#L3519-L3526

in ThreadSuspend::SuspendRuntime where it enumerates all the threads it is about to suspend.

I'm not sure how long this has been an issue, but I'm guessing it has been since this logic went in (~4 years ago circa .NET Core 2.2).

Is there another way we could transmit this information? We could save the 4 bytes _per_ sample event if we do away with this payload. That can add up fast when you have ~1000 samples _per_ thread _per_ second. That's roughly 4kb/s for a value that will _always_ be 1 or 2. Could we intern this information directly into the stack events? That would require a format change, though.

CC @sywhang @noahfalk @brianrob @tommcdon

Author:	josalem
Assignees:	-
Labels:	`area-Diagnostics-coreclr`
Milestone:	6.0.0

[brianrob]

Good catch @josalem. I suspect you are right that this has been this way since 2.2, as the initial EventPipe implementation in 2.0 was limited to UNIX which uses a different suspension mechanism.