-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Put LogExceptionThrown
on the NativeRuntimeEventSource
plan
#118729
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
Conversation
This unifies `LogExceptionThrown` with the other eventing that is implemented in the native runtime (threads/threadpool). Noticed this in dotnet#107418 (comment) where we run an awful lot of code in situations where event source is not even enabled.
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
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.
Pull Request Overview
This PR unifies the exception logging mechanism with the native runtime event system to improve performance by avoiding unnecessary code execution when event sources are disabled. The change moves from a direct runtime import call to using the NativeRuntimeEventSource pattern, which includes proper event source enablement checks.
Key Changes:
- Implements
ExceptionThrown_V1
event in theNativeRuntimeEventSource
with proper enablement checks - Replaces direct runtime import calls with the new event source method
- Updates the native runtime implementation to match the new signature
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
NativeRuntimeEventSource.Exceptions.NativeSinks.cs | Adds the main ExceptionThrown_V1 event method with enablement check for NativeAOT |
NativeRuntimeEventSource.Exceptions.NativeSinks.Internal.cs | Defines the LibraryImport for the native QCall implementation |
System.Private.CoreLib.Shared.projitems | Registers the new exception event files in the build system |
NativeRuntimeEventSourceGenerator.cs | Excludes the manually implemented exception event from code generation |
RuntimeImports.cs | Removes the old direct runtime import for exception logging |
Exception.NativeAot.cs | Updates exception logging to use the new event source method with enablement check |
runtimeeventinternal.cpp | Updates native implementation signature to include flags and ClrInstanceID parameters |
disabledruntimeeventinternal.cpp | Updates disabled runtime signature to match the new parameters |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
...te.CoreLib/src/System/Diagnostics/Tracing/NativeRuntimeEventSource.Exceptions.NativeSinks.cs
Show resolved
Hide resolved
...te.CoreLib/src/System/Diagnostics/Tracing/NativeRuntimeEventSource.Exceptions.NativeSinks.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/gen/NativeRuntimeEventSourceGenerator.cs
Outdated
Show resolved
Hide resolved
...te.CoreLib/src/System/Diagnostics/Tracing/NativeRuntimeEventSource.Exceptions.NativeSinks.cs
Outdated
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.
Thanks!
This unifies
LogExceptionThrown
with the other eventing that is implemented in the native runtime (threads/threadpool).Noticed this in #107418 (comment) where we run an awful lot of code in situations where event source is not even enabled.
Cc @dotnet/ilc-contrib