Fix trimming for DiagnosticSource #106680
Conversation
Recent changes to EventSource startup caused the IL trimmer to include portions of the DiagnosticSource assembly. Adding the IsMeterSupported feature check wasn't sufficient because EventSource also roots all of its methods via a DynamicallyAccessedMembers attribute. To ensure that the new methods can be removed when needed I refactored them into a separate EventSourceInitHelper class that won't be rooted by the existing DAM attribute. When EventSouce.IsSupported is false I'd expect the entire EventSourceInitHelper class to be unreachable. If only EventSource.IsMeterSupported is false then I'd expect PreregisterEventProviders and GetMetricsEventSource() to be unreachable but the rest of the class will remain. Hopefully this really fixes dotnet#106265 this time.
|
@MichalStrehovsky Vitek, in the issue you also mentioned "So I found first bug - the substitution doesn't define default value, so if the feature switch is not defined at all (which is the default), no substitution happens. We'll have to add featuredefaultvalue to the XML to fix this." My current PR doesn't have any changes to the XML and it wasn't clear what was needed. I've never messed with 'featuredefault' before, my understanding so far is just cobbled together from looking at other examples and I found the ILLinker code which I think processes it. From what I can tell other switches as well as the 'System.Diagnostics.Metrics.Meter.IsSupported' switch used from the DiagnosticSource assembly are already configured to act as though unspecified is the same as enabled. So it seemed like the new XML I previously added is also consistent with that pattern. If something should change there please let me know! |
I think this will. I've run rt-sz measurements on this PR and the results are: (Look at the labels to see what each one is measuring - MichalStrehovsky/rt-sz#66 is the most interesting because it's EventSourceSupport=true MetricsSupport=false and it's showing 150-300 kB savings across the board.) |
Sorry this is outdated. I thought we didn't want the feature to be enabled by default, since I've learned that is not the case for desktop - at least right now. So you don't need |
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
|
Thanks for the review and feedback. All the suggestions have been applied. |
|
I validated that this resolves the regression on the app where we diagnosed it with Matous originally (iOS sample mono app in runtime repo). |
Glad to hear it 👍 |
|
/backport to release/9.0 |
|
Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10516341520 |
|
Visible startup time improvement after this change.
|
Recent changes to EventSource startup caused the IL trimmer to include portions of the DiagnosticSource assembly. Adding the IsMeterSupported feature check wasn't sufficient because EventSource also roots all of its methods via a DynamicallyAccessedMembers attribute. To ensure that the new methods can be removed when needed I refactored them into a separate EventSourceInitHelper class that won't be rooted by the existing DAM attribute.
When EventSouce.IsSupported is false I'd expect the entire EventSourceInitHelper class to be unreachable. If only EventSource.IsMeterSupported is false then I'd expect PreregisterEventProviders and GetMetricsEventSource() to be unreachable but the rest of the class will remain.
Hopefully this really fixes #106265 this time.