Skip to content
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

[Xamarin.Android.Build.Tasks] feature switch defaults for fast startup #8347

Merged
merged 9 commits into from
Sep 21, 2023

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Sep 18, 2023

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 ()

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 ()
Comment on lines -2057 to -2059
void System.Diagnostics.Metrics.MeterListener:.cctor ()
void System.Diagnostics.Metrics.MetricsEventSource:.cctor ()
void System.Diagnostics.Metrics.MetricsEventSource:.ctor ()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! A decent number of these are gone now.

stdErr: Error: apkdiff: File 'assemblies/System.Diagnostics.DiagnosticSource.dll' has changed by -3,463 bytes (-38.24 %). This exceeds the threshold of 5.00 %.
Error: apkdiff: Size regression occured, 1 check(s) failed.
Comment on lines 55 to 57
"assemblies/System.Diagnostics.DiagnosticSource.dll": {
"Size": 12520
"Size": 9057
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a nice bonus:

stdErr: Error: apkdiff: File 'assemblies/System.Diagnostics.DiagnosticSource.dll' has changed by -3,463 bytes (-38.24 %). This exceeds the threshold of 5.00 %.
Error: apkdiff: Size regression occured, 1 check(s) failed.

@jonathanpeppers jonathanpeppers marked this pull request as ready for review September 19, 2023 15:21
@jonpryor
Copy link
Member

wip commit message:

Context: https://github.com/dotnet/runtime/commit/fecf3eeffd3650566555e15292f9df0d3abcdfc6
Context: https://github.com/dotnet/runtime/commit/3aeefbdd5e975e287effaa8670422837dc661d68
Context: https://github.com/dotnet/runtime/issues/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 ()

@@ -38,7 +38,17 @@ See: https://github.com/dotnet/runtime/blob/b13715b6984889a709ba29ea8a1961db469f
Trim="true" />
<!-- https://github.com/dotnet/runtime/blob/211cdd011f19a51b7092d8365e11e774a8280afb/src/libraries/System.Private.CoreLib/src/System/LocalAppContextSwitches.cs#L52 -->
<RuntimeHostConfigurationOption Include="Switch.System.Reflection.ForceInterpretedInvoke"
Value="$(_SystemReflectionForceInterpretedInvoke)"
Value="$(AndroidAvoidEmitForPerformance)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gives me some pause: Do we want/need/should we allow Switch.System.Reflection.ForceInterpretedInvoke and Microsoft.Extensions.DependencyInjection.DisableDynamicEngine to be handled separately?

$(_SystemDiagnosticsMetricsMeterIsSupported) allows System.Diagnostics.Metrics.Meter.IsSupported to be handled separately.

I wonder if we should leave this line unchanged -- continue using Value="$(_SystemReflectionForceInterpretedInvoke)" -- and also have the default values of $(_SystemReflectionForceInterpretedInvoke) and $(_SystemReflectionForceInterpretedInvoke) be the opposite of $(AndroidAvoidEmitForPerformance).

(Aside: how concerned should I be that one of these switches has a Switch. prefix, while the other doesn't?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reason to put both under a single property is simplicity. I can't really think of a good reason to toggle them at all -- let alone independently.

(Aside: how concerned should I be that one of these switches has a Switch. prefix, while the other doesn't?)

Switch. seems weird to me, too, but they have a mix of both in this file:

https://github.com/dotnet/runtime/blob/a5b75b8179d7db4ff720e19233d2b99fd15b94f0/src/libraries/System.Private.CoreLib/src/System/LocalAppContextSwitches.cs

I think it depends on which team implemented it...

@jonpryor jonpryor merged commit eb6397d into dotnet:main Sep 21, 2023
43 of 47 checks passed
@jonathanpeppers jonathanpeppers deleted the AvoidEmitForPerformance branch September 21, 2023 15:15
grendello added a commit to grendello/xamarin-android that referenced this pull request Sep 21, 2023
* main:
  [Xamarin.Android.Build.Tasks] feature switch defaults for fast startup (dotnet#8347)
  Localized file check-in by OneLocBuild Task (dotnet#8358)
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants