[Xamarin.Android] Performance regression between net7 and net8 #89880
Comments
|
Tagging subscribers to 'arch-android': @steveisok, @akoeplinger Issue DetailsWe've observed that a plain "hello world" Xamarin.Android application (
XA uses profiled AOT to improve startup time, and recording a new profile allowed me to see what new methods The worrying parts are the HTTP requests, the
|
|
/cc @SamMonoRT |
|
The profile is recorded with this application https://github.com/xamarin/xamarin-android/blob/main/src/profiled-aot/CommonMethods.cs This explains the TPL and the HTTP, luckily :) The TZ requests and the metrics handler are still worrying. |
|
@antonfirsov - can #87319 cause a startup regression. @grendello is it possible to coordinate with @antonfirsov to run some profiles before/after the change. |
|
/cc @MihaZupan @karelz and also @tarekgh in case this will be root caused to Meter performance. @grendello since we don't have experience with Android, can you help us by running profiler/benchmarks? |
If |
|
@antonfirsov if you point me to the commits I need to test, I can do the rest :) Note that this is not part of the startup for a hello world app, but the AOT profile recording app linked to above. However, any non-trivial app is likely to hit the same code path, so it would be great if we could address that. |
|
@antonfirsov it would be ideal if metrics were disabled by default, but easily enabled by setting an msbuild property of something. On mobile, it's best to do as little as possible by default. |
If we suspect metrics, you need to check Note that If this has to be revisited, and we need global switch(es) to turn the entire feature on/off (per library? for the entire BCL?) you need to discuss it with @noahfalk. |
|
When nothing is listening to metrics (the default), the overhead on an individual request should be negligible (we'll do a quick check to see if anything is listening and then skip all the extra logic). As Anton said though, someone may start collecting this information at any time, so the HttpClient instrumentation has to do some work at startup to prepare for that possibility. 28a4c95 was the initial commit that added this logic to HttpClient. There have been a few changes to that logic in main since, but they shouldn't meaningfully affect startup costs. |
|
@MihaZupan the JIT still needs to compile the methods (or load them from AOT), initialize the class etc. On mobile every bit helps, and if you have dozens or hundreds of things with negligible impact, then taken together they will have a huge impact. We've been through that process in Xamarin.Android and we managed to get our startup time from 1.6s to 120ms back then - without a single large offender, just hundreds and hundreds tiny things. This is why I'm always very insistent on not doing anything by default unless the app cannot properly run without it. |
|
I believe the extra cost here is coming from http client enabling the metrics. Looking at the description and the list of called methods, looks to me the cost is because enabling metrics first time will start do some static initialization These initializations were not exercised before in the app startup scenario like the one mentioned here. So, enabling it will be expected to add some cost to the app startup. @antonfirsov is it possible you can make the metrics initialization in http be lazy? to avoid the cost during the app startup. Or changing the app to not depend on the http handler in the startup scenario. |
|
@tarekgh static state was one of the offenders we had when optimizing Xamarin.Android startup. On mobile it's best to avoid that as much as possible. Going lazy is the way to go! :) |
If I prepare a draft change for that, would you be able to test it @grendello? |
I don't see how. My understanding is that one cannot opt-in into listening without creating the meters and instruments first. What am I missing? This is weird, I wonder what is starting a listener in the test app? |
|
We could make it lazy up until the first request (edit: we already do this), but yeah after that we'll need the |
The listener is not started, it is just statically initialized. no listener object is created as I am seeing. |
Absolutely, but not earlier than on Monday :) |
I think the only option to prevent startup impact is to make it opt-in or opt out. |
Yeah the handler chain initialization is lazy. I think there should be a global switch to turn built-in metrics on/off for the entire BCL, which should be off by default for mobile. You should reach out to @noahfalk, come up with a design, then @MihaZupan can raise a PR to implement it for |
That would be ideal |
|
(Still would be nice to see some numbers to confirm the root-causing.) |
I think there is some misunderstanding here. Creating metrics is cheap in general cases. The cost only be when someone start listening to the metrics. The issue reported here is scoped to the app startup which causes initializing the metrics first time in the process. Having http do lazy initialization should help with that. |
As @grendello mentioned, this is not happening in the test app: https://github.com/xamarin/xamarin-android/blob/main/src/profiled-aot/CommonMethods.cs
We already do that. The test app makes an HTTP request, which involves instrument creation. Without creating the instruments, there is no way to enable listening. @grendello I think it would be valuable to do some benchmarking/profiling and elaborate what exactly is causing the slowdown. |
|
@steveisok for the sdk question ^^ |
|
@jonathanpeppers did you already bring this up to the sdk? |
|
@grendello is out, but I will check back with him next week, what the state of the The feature switch added in #91352 for Microsoft.Extensions, we are going to implement in the Android workload as soon as we have a runtime build to try it. |
Not yet. #89880 (comment). I am planning to start working on that this week. Is it possible when I have a private build or changes to get some help measuring the Android perf to ensure the changes are going to address the raised issue? To be clearer, we are going to introduce a feature switch to the metrics APIs and not for http client. |
|
I basically just did this in a .NET MAUI app to test my Microsoft.Extensions changes:
I had a local build in But if your changes are in the BCL, this simple option might not work. You can also try: |
|
I need to do the exact measurements that @grendello did #89880 (comment). |
|
cc @kg @pavelsavara re startup regressions |
I'll try to get the numbers in the next couple of days (been out for 2 weeks, need to get up to speed with all that's happened first) |
|
Thanks @grendello, could you please answer @eerhardt question in the comment #89880 (comment) too? |
Yes, the results in the OP were obtained with a fresh AOT profile which included the metrics code. |
|
@tarekgh when you have the changes you'd like to measure impact of I can test them for you, if you point me to a dotnet/runtime branch or give me a diff of the changes. I can build these locally and run the tests. Alternatively, if you want to be able to iterate quickly, you could build XA yourself and use my performance test utility to run the tests. However, getting your new BCL to be used in an XA app build requires some |
|
@grendello I'll share the changes with you when I have them. |
|
@grendello as we chatted offline, did you have a chance to try #91767 and measure it with your scenario? I appreciate your help with that. We have a few days to get this in .NET 8.0, otherwise this will not make it if delayed. |
|
@tarekgh not yet, but I will get to it in a 2-3 hours. Will report on the results before EOD today. |
|
@tarekgh good news! Disabling metrics with the feature flag shows a ~4% improvement in total startup time, and a similar amount for the managed code init time :) |
|
Thanks a lot @grendello . I'll go ahead and proceed with merging the changes. |
|
Excellent, thank you for addressing the issue so quickly :) |
ghost
added
the
in-pr
ghost
removed
the
in-pr
|
This is now completed. |
Context: dotnet/runtime@fecf3ee Context: dotnet/runtime@3aeefbd Context: dotnet/runtime#89880 In dotnet/runtime two new feature switches were added in order to improve startup performance on Android: * `System.Diagnostics.Metrics.Meter.IsSupported`: disables `System.Diagnostics.Metrics` support for `HttpClient`, etc. * `Microsoft.Extensions.DependencyInjection.DisableDynamicEngine`: after the second call to retrieve a service from a dependency injection container, `System.Reflection.Emit is used to improve performance of repeated calls. We also have the existing switch: * `Switch.System.Reflection.ForceInterpretedInvoke`: after the second call to `MethodInfo.Invoke()` or `ConstructorInfo.Invoke()`, `System.Reflection.Emit is used to improve performance of repeated calls. Introduce two new MSBuild properties: * `$(AvoidEmitForPerformance)` to toggle both `Microsoft.Extensions.DependencyInjection.DisableDynamicEngine` and `Switch.System.Reflection.ForceInterpretedInvoke`. This is a public property that defaults to `true`, matching a property of the same name used somewhere in ASP.NET. * `$(_SystemDiagnosticsMetricsMeterIsSupported)` to toggle `System.Diagnostics.Metrics.Meter.IsSupported`. This is a private property that defaults to `false`. I added a test to check `System.Diagnostics.Metrics.Meter.IsSupported` at runtime. I did not do this with Microsoft.Extensions, as we'd have to add a reference to the NuGet package and track its version for nightly builds. I also updated the AOT profile, and some `System.Diagnostics.Metrics.Meter` API calls are gone now: bool System.Diagnostics.Metrics.Meter:AddInstrument (System.Diagnostics.Metrics.Instrument) object System.Diagnostics.Metrics.Instrument:get_SyncObject () System.Diagnostics.Metrics.Counter`1<long> System.Diagnostics.Metrics.Meter:CreateCounter (string,string,string,System.Collections.Generic.IEnumerable`1<System.Collections.Generic.KeyValuePair`2<string, object>>) System.Diagnostics.Metrics.Counter`1<long> System.Diagnostics.Metrics.Meter:CreateCounter (string,string,string) void System.Diagnostics.Metrics.MeterListener:.cctor () void System.Diagnostics.Metrics.MetricsEventSource:.cctor () void System.Diagnostics.Metrics.MetricsEventSource:.ctor ()
#8347) Context: dotnet/runtime@fecf3ee Context: dotnet/runtime@3aeefbd Context: dotnet/runtime#89880 In dotnet/runtime two new feature switches were added in order to improve startup performance on Android: * `System.Diagnostics.Metrics.Meter.IsSupported`: disables `System.Diagnostics.Metrics` support for `HttpClient`, etc. * `Microsoft.Extensions.DependencyInjection.DisableDynamicEngine`: after the second call to retrieve a service from a dependency injection container, `System.Reflection.Emit` is used to improve performance of repeated calls. We also have the existing switch: * `Switch.System.Reflection.ForceInterpretedInvoke`: after the second call to `MethodInfo.Invoke()` or `ConstructorInfo.Invoke()`, `System.Reflection.Emit` is used to improve performance of repeated calls. Introduce two new MSBuild properties: * `$(AndroidAvoidEmitForPerformance)` to toggle both `Microsoft.Extensions.DependencyInjection.DisableDynamicEngine` and `Switch.System.Reflection.ForceInterpretedInvoke`. This is a public property that defaults to `true`. * `$(_SystemDiagnosticsMetricsMeterIsSupported)` to toggle `System.Diagnostics.Metrics.Meter.IsSupported`. This is a private property that defaults to `false`. I added a test to check `System.Diagnostics.Metrics.Meter.IsSupported` at runtime. I did not do this with Microsoft.Extensions, as we'd have to add a reference to the NuGet package and track its version for nightly builds. I also updated the AOT profile, and some `System.Diagnostics.Metrics.Meter` API calls are now gone: bool System.Diagnostics.Metrics.Meter:AddInstrument (System.Diagnostics.Metrics.Instrument) object System.Diagnostics.Metrics.Instrument:get_SyncObject () System.Diagnostics.Metrics.Counter`1<long> System.Diagnostics.Metrics.Meter:CreateCounter (string,string,string,System.Collections.Generic.IEnumerable`1<System.Collections.Generic.KeyValuePair`2<string, object>>) System.Diagnostics.Metrics.Counter`1<long> System.Diagnostics.Metrics.Meter:CreateCounter (string,string,string) void System.Diagnostics.Metrics.MeterListener:.cctor () void System.Diagnostics.Metrics.MetricsEventSource:.cctor () void System.Diagnostics.Metrics.MetricsEventSource:.ctor ()
ghost
locked as resolved and limited conversation to collaborators
We've observed that a plain "hello world" Xamarin.Android application (
dotnet new android) starts up slowerthan it did under net7. The slowdown cannot be attribute to changes in XA itself, as there haven't been any
changes that could affect performance to this degree. Our performance test measures three startup metrics:
time between starting the application (via intent or by clicking its icon in the UI) and the point where
the application frame is fully painted. This metric regressed by an average of 11% across 10 test runs.
This metric regressed by around 50%
sequence above, plus MonoVM initialization, loading of some assemblies as needed to make the first managed
call, setting up handlers etc. This metric regressed by about 25%
XA uses profiled AOT to improve startup time, and recording a new profile allowed me to see what new methods
are called at startup. Here's a set of them that has the biggest potential to adversely affect performance:
The worrying parts are the HTTP requests, the
System.Diagnostic.Metricscalls, the TPL use and the timezone data - the latter was greatly improved lately, but it appears that accessing timezone data has somehow become part of the default startup path.startup (the biggest issue here is retrieving the zone's display name, which is an expensive operation)
The text was updated successfully, but these errors were encountered: