-
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
[mono][size][Perf] iOS and Android size regression on 8/9/2024 8:00:01 AM #106265
Comments
Tagging subscribers to this area: @directhex, @matouskozak |
Tagging subscribers to 'os-ios': @vitek-karas, @kotlarmilos, @ivanpovazan, @steveisok, @akoeplinger |
Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar |
The range of regression is 105bf4d...68511fd. I did manual verification and the causing commit is #106014 @noahfalk The same range is causing ~100kB size regression on Android sample app as well |
Tagging subscribers to 'arch-android': @vitek-karas, @simonrozsival, @steveisok, @akoeplinger |
@noahfalk @tarekgh the #106415 didn't have impact on Android/iOS size as expected fyi: @vitek-karas |
What diagnostics are available to determine the APIs that are no longer being trimmed and the rooting path preventing their removal? I am unfamiliar with how these size issues should be diagnosed. |
If you know which API was left in the binary and shouldn't have been then there are several ways to tell why it was kept:
To figure out which API is there and shouldn't be is typically a much more difficult problem and requires domain knowledge. For NativeAOT we can use https://github.com/MichalStrehovsky/sizoscope to diff two binaries, but trimmer doesn't support that yet. |
I can make guesses that the APIs causing the size increase are either EventSource APIs or Meter APIs. I don't know how to verify those guesses. I'm not sure what app this size measuring automation is building and how I would reproduce it locally. |
I'm trying to get the IL of the trimmed app - before/after - then we'll have something to work with. |
So the comparison is pretty clear: I'll look into it some more this happens and where it came from. |
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. Unfortunately even if I work around this I still can't get the app to smaller size. I can see the substitution happening correctly, but it's as if something else pulls in the code as well - still looking (first time doing some real work on a mac ... and it hurts :-)) |
As far as I know the only link from S.P.CoreLib -> DiagnosticSource is the Type.GetType() call here. That call is supposed to be guarded by both the |
OK - I think I understand this fully. The behavior is the same on Windows as well BTW. If I compare P6 and RC1 - win-x64 NativeAOT (so not just normal trimming)
So we regressed the size of console hello world by 75KB, so by almost 6%. The reason is the same, we now include parts of DefaultsCurrently both Trimming problemIf I turn off To fix this, the cleanest solution would be to create a separate type from Side note: The fact that Acceptable size regression?The second interesting question is - Do we need/want to enable metrics by default - on Windows console, on iOS, on other targets. I don't know what functionality it provides and if it's something every app should have enabled by default. @noahfalk maybe you could point us to some reading on this, so that we can decide if it makes sense to enable it for mobile targets. I'll leave the Windows/Linux/macOS targets up to you :-) Measuring the right stuffAFAIK we're not aware of NativeAOT size regression due to this change - right @MichalStrehovsky . That raises the question which infra should have caught it and why it didn't. We'll have to live with this for RC1, but we should try to answer all these questions for RC2. |
Good catch with the I think moving that method out into a separate type sounds like a good fix. I'm not seeing the size regression for native AOT hello world on Windows. I see 1,104,384 bytes with P6 on a We default EventSourceSupport to I do see regression on a console hello world with It would be nice if someone who understands metrics to confirm the 300 kB regression is expected. Download and unpack this zip: Then a Windows machine, run Notice the nodes in a different color only exists in "compare", not in "baseline". So for example in this case a new |
Thanks for investigating!
Can the separate type be an inner class or will DAM.All pull that in too? I'm fine to do it as a top-level class if necessary, but inner class seemed a little nicer if the only point of the class is to get the trimmer behavior we want.
These docs should give a good idea what scenarios you lose out on if you disable Meter:
Metrics are most prevalent in web-services, but they can also be useful for ad-hoc performance investigations in all types of apps. In terms of defaults, my initial thought is that turning on/off EventSource seems like a better lever to use than Meter. If you are planning to turn Meter off by default and leave EventSource on I'd be curious to better understand the rationale.
According to sizoscope it showed me the regression as 250.4 rather than 300, but aside from that yeah, the new dependencies from metrics seemed right. |
Yeah, size optimization for instrumentation has never been a goal we've prioritized. The feedback that I see has been mostly about adding new capabilities, integration, standardization and performance at runtime. In many cases I suspect the perf at runtime has direct tradeoffs against code size as the fastest/lowest alloc implementations tend to be heavier on custom structs and generic instantiations of code to process them. I'm happy to chat more about it any time. |
Sizoscope doesn't have exact accounting of things - it pretty much always underreports because some bytes of the output are too difficult to attribute but those bytes are generally related to things that are visible in the UI. Thank you for checking! |
This would need to be a separate class - runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs Lines 3141 to 3159 in 0fca85f
|
Haha, I didn't even realize inner class reflection was in EventSource's bag of tricks. I'll have to tuck that tid-bit away somewhere. Thanks for the info! I'll put together another change that moves the code into a separate top-level internal class soon, hopefully tomorrow. |
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.
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.
* Fix trimming for DiagnosticSource 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. * PR feedback --------- Co-authored-by: Noah Falk <[email protected]> Co-authored-by: Jeff Schwartz <[email protected]>
Run Information
Regressions in SOD - iOS HelloWorld Mono .app Size llvm nosymbols
Test Report
Repro
General Docs link: https://github.com/dotnet/performance/blob/main/docs/benchmarking-workflow-dotnet-runtime.md
Repro Steps
Prerequisites (Build files either built locally or downloaded from payload above)
runtime/artifacts
or build instructions: Libraries README args:-subset libs+libs.tests -rc release -configuration Release -arch $RunArch -framework net8.0
runtime/artifacts/bin/coreclr/$RunOS.$RunArch.Release
, build instructions: CoreCLR README args:-subset clr+libs -rc release -configuration Release -arch $RunArch -framework net8.0
runtime/artifacts/bin/mono/$RunOS.$RunArch.Release
, build instructions: MONO README args:-arch $RunArch -os $RunOS -s mono+libs+host+packs -c Release
Linux
Windows
SOD - iOS HelloWorld Mono .app Size llvm nosymbols
ETL Files
Histogram
JIT Disasms
Docs
Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository
Run Information
Regressions in SOD - iOS HelloWorld Mono Zip Size llvm nosymbols
Test Report
Repro
General Docs link: https://github.com/dotnet/performance/blob/main/docs/benchmarking-workflow-dotnet-runtime.md
Repro Steps
Prerequisites (Build files either built locally or downloaded from payload above)
runtime/artifacts
or build instructions: Libraries README args:-subset libs+libs.tests -rc release -configuration Release -arch $RunArch -framework net8.0
runtime/artifacts/bin/coreclr/$RunOS.$RunArch.Release
, build instructions: CoreCLR README args:-subset clr+libs -rc release -configuration Release -arch $RunArch -framework net8.0
runtime/artifacts/bin/mono/$RunOS.$RunArch.Release
, build instructions: MONO README args:-arch $RunArch -os $RunOS -s mono+libs+host+packs -c Release
Linux
Windows
SOD - iOS HelloWorld Mono Zip Size llvm nosymbols
ETL Files
Histogram
JIT Disasms
Docs
Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository
The text was updated successfully, but these errors were encountered: