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

[DependencyInjection] introduce feature switch to disable S.R.E #91133

Conversation

jonathanpeppers
Copy link
Member

When recording a new AOT profile for .NET MAUI apps running on Android, we noticed that System.Reflection.Emit work was being done on a background thread. The call seen in dotnet-trace output:

11.32ms microsoft.extensions.dependencyinjection!Microsoft.Extensions.DependencyInjection.ServiceLookup.ILEmitResolverBuilder.GenerateMethodBody(Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceCallSite,System.Reflection.Emit.ILGenerator)

.NET Android apps are unique in that there is a JIT, RuntimeFeature.IsDynamicCodeCompiled is true, System.Reflection.Emit is possible -- S.R.E is however, not great for startup performance.

Starting threads on Android during startup can also be slow, as Android will commonly put all but a single core to sleep for battery saving purposes. We try to avoid starting threads on startup for "hello world" applications on Android.

To solve this for now, introduce a new feature flag:

Microsoft.Extensions.DependencyInjection.DisableDynamicEngine

Which, we will provide a value in either the Android or .NET MAUI optional workload via an MSBuild property. To test, I put this in my app's .csproj file:

<RuntimeHostConfigurationOption Include="Microsoft.Extensions.DependencyInjection.DisableDynamicEngine"
                                Condition="'$(DisableDynamicEngine)' != ''"
                                Value="$(DisableDynamicEngine)"
                                Trim="true" />

Customers could opt to change this flag, but we don't think it will particularly useful. An example of services realized by .NET MAUI at startup, via some logging added:

08-25 13:21:55.647 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.IMauiInitializeService]
08-25 13:21:55.664 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.IMauiInitializeScopedService]
08-25 13:21:55.665 16530 16530 I DOTNET  : RealizeService called: Microsoft.Maui.Dispatching.IDispatcher
08-25 13:21:55.668 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.LifecycleEvents.LifecycleEventRegistration]
08-25 13:21:56.057 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.HandlerMauiAppBuilderExtensions+HandlerRegistration]
08-25 13:21:56.115 16530 16530 I DOTNET  : RealizeService called: Microsoft.Extensions.DependencyInjection.IServiceScopeFactory
08-25 13:21:56.670 16530 16530 I DOTNET  : RealizeService called: Microsoft.Maui.Controls.HideSoftInputOnTappedChangedManager
08-25 13:21:56.712 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.ImageSourcesMauiAppBuilderExtensions+ImageSourceRegistration]
08-25 13:21:57.700 16530 16530 I DOTNET  : RealizeService using S.R.E: Microsoft.Maui.Controls.HideSoftInputOnTappedChangedManager

HideSoftInputOnTappedChangedManager would be realized once per page, which would not be a huge payoff to use S.R.E for. So the only way the S.R.E codepath could be useful on Android would be if the customer is registering lots of services themselves. They might be better off just using new() in that case?

An example of the startup time Android reports with the new flag on/off:

DisableDynamicEngine=false
08-25 14:31:37.462  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +733ms
08-25 14:31:39.394  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +737ms
08-25 14:31:41.326  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +730ms
DisableDynamicEngine=true
08-25 14:32:20.233  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +724ms
08-25 14:32:22.137  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +727ms
08-25 14:32:24.042  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +716ms

This was a dotnet new maui project, using dotnet/maui/main on a Pixel 5 device.

When recording a new AOT profile for .NET MAUI apps running on Android,
we noticed that System.Reflection.Emit work was being done on a
background thread. The call seen in `dotnet-trace` output:

    11.32ms microsoft.extensions.dependencyinjection!Microsoft.Extensions.DependencyInjection.ServiceLookup.ILEmitResolverBuilder.GenerateMethodBody(Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceCallSite,System.Reflection.Emit.ILGenerator)

.NET Android apps are unique in that there is a JIT,
`RuntimeFeature.IsDynamicCodeCompiled` is true, System.Reflection.Emit
is possible -- S.R.E is however, not great for startup performance.

Starting threads on Android during startup can also be slow, as Android
will commonly put all but a single core to sleep for battery saving
purposes. We try to avoid starting threads on startup for "hello world"
applications on Android.

To solve this for now, introduce a new feature flag:

    Microsoft.Extensions.DependencyInjection.DisableDynamicEngine

Which, we will provide a value in either the Android or .NET MAUI
optional workload via an MSBuild property. To test, I put this in my
app's `.csproj` file:

    <RuntimeHostConfigurationOption Include="Microsoft.Extensions.DependencyInjection.DisableDynamicEngine"
                                    Condition="'$(DisableDynamicEngine)' != ''"
                                    Value="$(DisableDynamicEngine)"
                                    Trim="true" />

Customers *could* opt to change this flag, but we don't think it will
particularly useful. An example of services realized by .NET MAUI at
startup, via some logging added:

    08-25 13:21:55.647 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.IMauiInitializeService]
    08-25 13:21:55.664 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.IMauiInitializeScopedService]
    08-25 13:21:55.665 16530 16530 I DOTNET  : RealizeService called: Microsoft.Maui.Dispatching.IDispatcher
    08-25 13:21:55.668 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.LifecycleEvents.LifecycleEventRegistration]
    08-25 13:21:56.057 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.HandlerMauiAppBuilderExtensions+HandlerRegistration]
    08-25 13:21:56.115 16530 16530 I DOTNET  : RealizeService called: Microsoft.Extensions.DependencyInjection.IServiceScopeFactory
    08-25 13:21:56.670 16530 16530 I DOTNET  : RealizeService called: Microsoft.Maui.Controls.HideSoftInputOnTappedChangedManager
    08-25 13:21:56.712 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.ImageSourcesMauiAppBuilderExtensions+ImageSourceRegistration]
    08-25 13:21:57.700 16530 16530 I DOTNET  : RealizeService using S.R.E: Microsoft.Maui.Controls.HideSoftInputOnTappedChangedManager

`HideSoftInputOnTappedChangedManager` would be realized once per page,
which would not be a huge payoff to use S.R.E for. So the only way the
S.R.E codepath could be useful on Android would be if the customer is
registering lots of services themselves. They might be better off just
using `new()` in that case?

An example of the startup time Android reports with the new flag on/off:

    DisableDynamicEngine=false
    08-25 14:31:37.462  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +733ms
    08-25 14:31:39.394  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +737ms
    08-25 14:31:41.326  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +730ms
    DisableDynamicEngine=true
    08-25 14:32:20.233  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +724ms
    08-25 14:32:22.137  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +727ms
    08-25 14:32:24.042  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +716ms

This was a `dotnet new maui` project, using dotnet/maui/main on a Pixel
5 device.
@ghost
Copy link

ghost commented Aug 25, 2023

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection
See info in area-owners.md if you want to be subscribed.

Issue Details

When recording a new AOT profile for .NET MAUI apps running on Android, we noticed that System.Reflection.Emit work was being done on a background thread. The call seen in dotnet-trace output:

11.32ms microsoft.extensions.dependencyinjection!Microsoft.Extensions.DependencyInjection.ServiceLookup.ILEmitResolverBuilder.GenerateMethodBody(Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceCallSite,System.Reflection.Emit.ILGenerator)

.NET Android apps are unique in that there is a JIT, RuntimeFeature.IsDynamicCodeCompiled is true, System.Reflection.Emit is possible -- S.R.E is however, not great for startup performance.

Starting threads on Android during startup can also be slow, as Android will commonly put all but a single core to sleep for battery saving purposes. We try to avoid starting threads on startup for "hello world" applications on Android.

To solve this for now, introduce a new feature flag:

Microsoft.Extensions.DependencyInjection.DisableDynamicEngine

Which, we will provide a value in either the Android or .NET MAUI optional workload via an MSBuild property. To test, I put this in my app's .csproj file:

<RuntimeHostConfigurationOption Include="Microsoft.Extensions.DependencyInjection.DisableDynamicEngine"
                                Condition="'$(DisableDynamicEngine)' != ''"
                                Value="$(DisableDynamicEngine)"
                                Trim="true" />

Customers could opt to change this flag, but we don't think it will particularly useful. An example of services realized by .NET MAUI at startup, via some logging added:

08-25 13:21:55.647 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.IMauiInitializeService]
08-25 13:21:55.664 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.IMauiInitializeScopedService]
08-25 13:21:55.665 16530 16530 I DOTNET  : RealizeService called: Microsoft.Maui.Dispatching.IDispatcher
08-25 13:21:55.668 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.LifecycleEvents.LifecycleEventRegistration]
08-25 13:21:56.057 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.HandlerMauiAppBuilderExtensions+HandlerRegistration]
08-25 13:21:56.115 16530 16530 I DOTNET  : RealizeService called: Microsoft.Extensions.DependencyInjection.IServiceScopeFactory
08-25 13:21:56.670 16530 16530 I DOTNET  : RealizeService called: Microsoft.Maui.Controls.HideSoftInputOnTappedChangedManager
08-25 13:21:56.712 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.ImageSourcesMauiAppBuilderExtensions+ImageSourceRegistration]
08-25 13:21:57.700 16530 16530 I DOTNET  : RealizeService using S.R.E: Microsoft.Maui.Controls.HideSoftInputOnTappedChangedManager

HideSoftInputOnTappedChangedManager would be realized once per page, which would not be a huge payoff to use S.R.E for. So the only way the S.R.E codepath could be useful on Android would be if the customer is registering lots of services themselves. They might be better off just using new() in that case?

An example of the startup time Android reports with the new flag on/off:

DisableDynamicEngine=false
08-25 14:31:37.462  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +733ms
08-25 14:31:39.394  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +737ms
08-25 14:31:41.326  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +730ms
DisableDynamicEngine=true
08-25 14:32:20.233  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +724ms
08-25 14:32:22.137  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +727ms
08-25 14:32:24.042  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +716ms

This was a dotnet new maui project, using dotnet/maui/main on a Pixel 5 device.

Author: jonathanpeppers
Assignees: jonathanpeppers
Labels:

area-Extensions-DependencyInjection

Milestone: -

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@@ -246,7 +249,7 @@ private ServiceProviderEngine GetEngine()
#if NETFRAMEWORK || NETSTANDARD2_0
engine = CreateDynamicEngine();
#else
if (RuntimeFeature.IsDynamicCodeCompiled)
if (RuntimeFeature.IsDynamicCodeCompiled && !DisableDynamicEngine)
Copy link
Member

Choose a reason for hiding this comment

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

For other reviewers: the check for IsDynamicCodeCompiled here was added prior to v8.

@steveharter
Copy link
Member

Note the related emit+Android issue where emit is also to be avoided for reflection invoke: #83893 which uses Switch.System.Reflection.ForceInterpretedInvoke.

Should the changes there that use ForceInterpretedInvoke be changed to use the new switch added here?

@steveharter steveharter added tenet-performance Performance related issue os-android labels Aug 29, 2023
@ghost
Copy link

ghost commented Aug 29, 2023

Tagging subscribers to 'arch-android': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

When recording a new AOT profile for .NET MAUI apps running on Android, we noticed that System.Reflection.Emit work was being done on a background thread. The call seen in dotnet-trace output:

11.32ms microsoft.extensions.dependencyinjection!Microsoft.Extensions.DependencyInjection.ServiceLookup.ILEmitResolverBuilder.GenerateMethodBody(Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceCallSite,System.Reflection.Emit.ILGenerator)

.NET Android apps are unique in that there is a JIT, RuntimeFeature.IsDynamicCodeCompiled is true, System.Reflection.Emit is possible -- S.R.E is however, not great for startup performance.

Starting threads on Android during startup can also be slow, as Android will commonly put all but a single core to sleep for battery saving purposes. We try to avoid starting threads on startup for "hello world" applications on Android.

To solve this for now, introduce a new feature flag:

Microsoft.Extensions.DependencyInjection.DisableDynamicEngine

Which, we will provide a value in either the Android or .NET MAUI optional workload via an MSBuild property. To test, I put this in my app's .csproj file:

<RuntimeHostConfigurationOption Include="Microsoft.Extensions.DependencyInjection.DisableDynamicEngine"
                                Condition="'$(DisableDynamicEngine)' != ''"
                                Value="$(DisableDynamicEngine)"
                                Trim="true" />

Customers could opt to change this flag, but we don't think it will particularly useful. An example of services realized by .NET MAUI at startup, via some logging added:

08-25 13:21:55.647 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.IMauiInitializeService]
08-25 13:21:55.664 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.IMauiInitializeScopedService]
08-25 13:21:55.665 16530 16530 I DOTNET  : RealizeService called: Microsoft.Maui.Dispatching.IDispatcher
08-25 13:21:55.668 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.LifecycleEvents.LifecycleEventRegistration]
08-25 13:21:56.057 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.HandlerMauiAppBuilderExtensions+HandlerRegistration]
08-25 13:21:56.115 16530 16530 I DOTNET  : RealizeService called: Microsoft.Extensions.DependencyInjection.IServiceScopeFactory
08-25 13:21:56.670 16530 16530 I DOTNET  : RealizeService called: Microsoft.Maui.Controls.HideSoftInputOnTappedChangedManager
08-25 13:21:56.712 16530 16530 I DOTNET  : RealizeService called: System.Collections.Generic.IEnumerable`1[Microsoft.Maui.Hosting.ImageSourcesMauiAppBuilderExtensions+ImageSourceRegistration]
08-25 13:21:57.700 16530 16530 I DOTNET  : RealizeService using S.R.E: Microsoft.Maui.Controls.HideSoftInputOnTappedChangedManager

HideSoftInputOnTappedChangedManager would be realized once per page, which would not be a huge payoff to use S.R.E for. So the only way the S.R.E codepath could be useful on Android would be if the customer is registering lots of services themselves. They might be better off just using new() in that case?

An example of the startup time Android reports with the new flag on/off:

DisableDynamicEngine=false
08-25 14:31:37.462  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +733ms
08-25 14:31:39.394  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +737ms
08-25 14:31:41.326  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +730ms
DisableDynamicEngine=true
08-25 14:32:20.233  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +724ms
08-25 14:32:22.137  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +727ms
08-25 14:32:24.042  2090  2330 I ActivityTaskManager: Displayed com.companyname.testmaui/crc643c09abdeec717b83.MainActivity: +716ms

This was a dotnet new maui project, using dotnet/maui/main on a Pixel 5 device.

Author: jonathanpeppers
Assignees: jonathanpeppers
Labels:

tenet-performance, os-android, area-Extensions-DependencyInjection

Milestone: -

@jonathanpeppers
Copy link
Member Author

My plan was to add changes to the Android workload in a similar way as: dotnet/android#7972

<DisableDependencyInjectionDynamicEngine Condition="'$(DisableDependencyInjectionDynamicEngine)' == ''">true</DisableDependencyInjectionDynamicEngine>

I wouldn't expect many of our customers to know about these or try to use them.

I can unify these both to use a single MSBuild property, though -- thoughts on a good name? $(DisableSystemReflectionEmitOptimizations)?

@@ -27,6 +27,7 @@ configurations but their defaults might vary as any SDK can set the defaults dif
| MetadataUpdaterSupport | System.Reflection.Metadata.MetadataUpdater.IsSupported | Metadata update related code to be trimmed when set to false |
| _EnableConsumingManagedCodeFromNativeHosting | System.Runtime.InteropServices.EnableConsumingManagedCodeFromNativeHosting | Getting a managed function from native hosting is disabled when set to false and related functionality can be trimmed. |
| VerifyDependencyInjectionOpenGenericServiceTrimmability | Microsoft.Extensions.DependencyInjection.VerifyOpenGenericServiceTrimmability | When set to true, DependencyInjection will verify trimming annotations applied to open generic services are correct |
| DisableDependencyInjectionDynamicEngine | Microsoft.Extensions.DependencyInjection.DisableDynamicEngine | When set to true, DependencyInjection will avoid using System.Reflection.Emit when realizing services |
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if a more universal "AvoidEmitForPerformance" switch makes sense. That would also be checked in the couple places where the undocumented ForceInterpretedInvoke is used (which was originally added for testing).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think it makes sense to rework dotnet/android#7972 so it sets both feature switches on a name similar to $(AvoidEmitForPerformance).

We would try to ship this in .NET 8 RC 2 if possible.

Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

Per offline discussion, we may want to remove the .md file changes since this switch is considered internal and may be replaced by "AvoidEmitForPerformance" (or a better name) in the future where that new switch would also by used by Reflection and DependencyInjection for emit cases where today they use IsDynamicCodeSupported.

@jonathanpeppers jonathanpeppers merged commit fecf3ee into dotnet:main Aug 30, 2023
104 of 106 checks passed
@jonathanpeppers jonathanpeppers deleted the Microsoft.Extensions.DependencyInjection.DisableDynamicEngine branch August 30, 2023 19:44
@jonathanpeppers
Copy link
Member Author

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6029373668

@github-actions
Copy link
Contributor

@jonathanpeppers an error occurred while backporting to release/8.0, please check the run log for details!

Error: @jonathanpeppers is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=jonathanpeppers

@steveharter
Copy link
Member

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6029397098

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants