-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add managed entry points for raising Contention events #87087
Conversation
kouvel
commented
Jun 3, 2023
- Added the events similarly to how the PortableThreadPool events are currently set up
- The events are not raised anywhere from the managed side yet, it's expected that they will eventually be raised by a new Lock type and at least by NativeAOT's Lock
Tagging subscribers to this area: @mangod9 Issue Details
|
Do we expect to raise these as well from SpinLock? What about other managed lock types, like ReaderWriterLockSlim? |
It would be nice to in the future, though I haven't seen much desire for it. For doing that though, could maybe consider adding an enum field that indicates what type caused the contention for easier filtering. Although the ContentionStart event has stacks, stack tracing can be missed sometimes. Or maybe they can be different events for each type, that would make it easier to identify quickly from a profile, but then there's also a question about whether Monitor.LockContentionCount should include contention from all of those types or if we should have different contention properties on the appropriate types, and what the dotnet-monitor contention count would show. My inclination currently is to evaluate expanding it based on need. |
...ate.CoreLib/src/System/Diagnostics/Tracing/NativeRuntimeEventSource.Threading.NativeSinks.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -1440,6 +1440,8 @@ | |||
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\Tracing\IncrementingEventCounter.cs" /> | |||
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\Tracing\IncrementingPollingCounter.cs" /> | |||
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\Tracing\NativeRuntimeEventSource.cs" /> | |||
<Compile Include="$(MSBuildThisFileDirectory)System\Diagnostics\Tracing\NativeRuntimeEventSource.Threading.cs" Condition="'$(FeaturePerfTracing)' != 'true' or '$(FeatureNativeAot)' == 'true'" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was not included previously if FeaturePortableThreadPool wasn't true and never on Mono/CoreCLR, now it looks like this will always be included if FeaturePerfTracing != 'true' and that is at least the case for TargetsBrowser (WASM).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously the file contained events that would only be raised by the portable thread pool. I reused the same file for contention events and they could be raised from anywhere. Some thread pool events too may be raised from different implementations in the future.
The condition that enables FeaturePortableThreadPool in Mono is currently similar to the one that enables FeaturePerfTracing. I believe the correct condition would be just '$(FeaturePerfTracing)' != 'true'
, as FeaturePerfTracing seems to be the one used to set up eventing (and using genRuntimeEventSources.py
). In NativeAOT, currently FeaturePerfTracing is true but the eventing stuff appears to not be set up yet, so I modified the conditions so that the same file as before would for now be included in NativeAOT.
- Added the events similarly to how the PortableThreadPool events are currently set up. They may need further tweaking to make them work from NativeAOT. - The events are not raised anywhere from the managed side yet, it's expected that they will eventually be raised by a new Lock type and at least by NativeAOT's Lock