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.

Sign up for GitHub

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

Jump to bottom

[Xamarin.Android.Build.Tasks] disable Switch.System.Reflection.ForceInterpretedInvoke #7972

Merged
merged 1 commit into from
Apr 21, 2023
Merged

[Xamarin.Android.Build.Tasks] disable Switch.System.Reflection.ForceInterpretedInvoke #7972

merged 1 commit into from
Apr 21, 2023

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Apr 19, 2023 •
edited
Loading

Fixes: dotnet/runtime#83893

In .NET 8, System.Reflection.ConstructorInfo/MethodInfo.Invoke() will call System.Reflection.Emit when called more than once. This impacts startup in mobile applications, so it may not be a desired feature.

Unfortunately, this appears to happen quite easily in Android apps, some examples:

The types of situations this happens:

  • You call a Java method from C# that returns a Java.Lang.Object subclass.

  • You override a Java method in C#, that has a Java.Lang.Object parameter.

To solve this problem, we can set:

<ItemGroup>
  <RuntimeHostConfigurationOption Include="Switch.System.Reflection.ForceInterpretedInvoke" Value="$(_SystemReflectionForceInterpretedInvoke)" Trim="true" />
</ItemGroup>

And we can set $(_SystemReflectionForceInterpretedInvoke) to test out the setting in various apps.

I added a test to verify the "private" switch is actually set.

I also updated the .aotprofile to verify that all System.Reflection.Emit code paths disappear from dotnet new android applications.

jonathanpeppers
jonathanpeppers commented Apr 19, 2023
View reviewed changes
src/profiled-aot/dotnet.aotprofile.txt
Comment on lines -2328 to -2345
void System.Reflection.Emit.DynamicMethod:.cctor ()
void System.Reflection.Emit.DynamicMethod:.ctor (string,System.Type,System.Type[],System.Reflection.Module,bool)
void System.Reflection.Emit.DynamicMethod:create_dynamic_method (System.Reflection.Emit.DynamicMethod,string,System.Reflection.MethodAttributes,System.Reflection.CallingConventions)
void System.Reflection.Emit.DynamicMethod:CreateDynMethod ()
void System.Reflection.Emit.DynamicMethod:Init (string,System.Reflection.MethodAttributes,System.Reflection.CallingConventions,System.Type,System.Type[],System.Type,System.Reflection.Module,bool,bool)
void System.Reflection.Emit.ILGenerator:.ctor (System.Reflection.Module,System.Reflection.Emit.ITokenGenerator,int)
void System.Reflection.Emit.ILGenerator:Emit (System.Reflection.Emit.OpCode,int)
void System.Reflection.Emit.ILGenerator:Emit (System.Reflection.Emit.OpCode,System.Reflection.ConstructorInfo)
void System.Reflection.Emit.ILGenerator:Emit (System.Reflection.Emit.OpCode,System.Reflection.FieldInfo)
void System.Reflection.Emit.ILGenerator:Emit (System.Reflection.Emit.OpCode,System.Reflection.MethodInfo)
void System.Reflection.Emit.ILGenerator:Emit (System.Reflection.Emit.OpCode,System.Type)
void System.Reflection.Emit.ILGenerator:Emit (System.Reflection.Emit.OpCode)
void System.Reflection.Emit.ILGenerator:emit_int (int)
void System.Reflection.Emit.ILGenerator:label_fixup (System.Reflection.MethodBase)
void System.Reflection.Emit.ILGenerator:ll_emit (System.Reflection.Emit.OpCode)
void System.Reflection.Emit.ILGenerator:make_room (int)
void System.Reflection.Emit.OpCodes:.cctor ()
void System.Reflection.Emit.RuntimeModuleBuilder:.cctor ()
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!

@pjcollins
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonathanpeppers
[Xamarin.Android.Build.Tasks] disable Switch.System.Reflection.ForceI…
0b53dca 
…nterpretedInvoke

Fixes: dotnet/runtime#83893

In .NET 8, `System.Reflection.ConstructorInfo/MethodInfo.Invoke()` will
call `System.Reflection.Emit` when called more than once. This impacts
startup in mobile applications, so it may not be a desired feature.

Unfortunately, this appears to happen quite easily in Android apps,
some examples:

* https://gist.github.com/ivanpovazan/2563ea9d2fea320e6425cfcc58da3ee5
* https://gist.github.com/ivanpovazan/d2546d4abad17900d4366cc29e1689b2

The types of situations this happens:

* You call a Java method from C# that returns a `Java.Lang.Object`
  subclass.

* You override a Java method in C#, that has a `Java.Lang.Object`
  parameter.

To solve this problem, we can set:

    <ItemGroup>
      <RuntimeHostConfigurationOption Include="Switch.System.Reflection.ForceInterpretedInvoke" Value="$(_SystemReflectionForceInterpretedInvoke)" Trim="true" />
    </ItemGroup>

And we can set `$(_SystemReflectionForceInterpretedInvoke)` to test
out the setting in various apps.

I added a test to verify the "private" switch is actually set.

I also updated the `.aotprofile` to verify that all
`System.Reflection.Emit` code paths disappear from `dotnet new
android` applications.
@jonathanpeppers jonathanpeppers force-pushed the Switch.System.Reflection.ForceInterpretedInvoke branch from 5ccd09d to 0b53dca Compare April 19, 2023 19:47
@jonathanpeppers
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor
Copy link
Member

Draft commit message:

Fixes: https://github.com/dotnet/runtime/issues/83893

Context: https://github.com/dotnet/runtime/pull/72717

In .NET 8, `System.Reflection.{ConstructorInfo,MethodInfo}.Invoke()`
will use `System.Reflection.Emit` when called more than once.
This impacts startup in mobile applications, so it may not be a
desirable feature.

Unfortunately, this appears to happen quite easily in Android apps;
some examples (using a custom dotnet/runtime build for extra output):

  * https://gist.github.com/ivanpovazan/2563ea9d2fea320e6425cfcc58da3ee5
  * https://gist.github.com/ivanpovazan/d2546d4abad17900d4366cc29e1689b2

The primary situation in which this happens is that all Java-originated
`Java.Lang.Object` subclass constructor invocations always hit
`ConstructorInfo.Invoke()`; see `TypeManager.Activate()`.

To solve this problem, we can set:

	<ItemGroup>
	  <RuntimeHostConfigurationOption
	      Include="Switch.System.Reflection.ForceInterpretedInvoke"
	      Value="$(_SystemReflectionForceInterpretedInvoke)"
	      Trim="true"
	  />
	</ItemGroup>

And we can set `$(_SystemReflectionForceInterpretedInvoke)` to test
out the setting in various apps.

I added a test to verify the "private" switch is actually set.

I also updated the `.aotprofile` to verify that all
`System.Reflection.Emit` code paths disappear from
`dotnet new android` applications.

@jonpryor jonpryor merged commit 5687506 into dotnet:main Apr 21, 2023
@jonathanpeppers jonathanpeppers deleted the Switch.System.Reflection.ForceInterpretedInvoke branch April 21, 2023 18:41
grendello added a commit to grendello/xamarin-android that referenced this pull request Apr 24, 2023
@grendello
Merge branch 'main' into blazor-hang
4b595a8 
* main:
  [Xamarin.Android.Build.Tasks] enable ForceInterpretedInvoke switch (dotnet#7972)
  [Mono.Android] Bind API-UpsideDownCake Beta 1 (dotnet#7980)
  Bump to xamarin/xamarin-android-tools/main@8bc07503 (dotnet#7863)
  [automation] Add 'xaSourcePath' to yaml so they can be used from monodroid. (dotnet#7978)
  Bump to dotnet/installer@16c10f8115 8.0.100-preview.4.23218.1 (dotnet#7969)
  [docs] Add UnitTest.md  (dotnet#7877)
  [ci] Suppress fork PR build warnings (dotnet#7973)
  [Xamarin.Android.Build.Tasks] Bump ZipFlushFilesLimit (dotnet#7957)
  Bump to dotnet/installer@3ca7ad1c79 8.0.100-preview.4.23211.1 (dotnet#7946)
  [CI] Allow passing xamarin-android checkout dir to nested templates. (dotnet#7961)
  [Xamarin.Android.Build.Tasks] Fix `-int.ToString()` for locales (dotnet#7941)
  [ci] Automatically retry failed apk-instrumentation tests. (dotnet#7963)
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this pull request Apr 28, 2023
@jonathanpeppers
[android] AOT profiles for .NET 8 Preview 4
30e3dc1 
Updated the profile, following the instructions in:

    src/ProfiledAot/README.md

Testing a `dotnet new maui` project on a Pixel 5:

    Before:
    Average(ms): 568
    Std Err(ms): 2.21610268514595
    Std Dev(ms): 7.00793201387621
    After:
    Average(ms): 548.5
    Std Err(ms): 2.65518360947035
    Std Dev(ms): 8.39642781187333

Most notably calls to System.Reflection.Emit are now gone, after we
landed:

dotnet/android#7972
@jonathanpeppers jonathanpeppers mentioned this pull request Apr 28, 2023
Merged
mattleibow pushed a commit to dotnet/maui that referenced this pull request Apr 29, 2023
@jonathanpeppers
[android] AOT profiles for .NET 8 Preview 4 (#14838)
f24e8da 
Updated the profile, following the instructions in:

    src/ProfiledAot/README.md

Testing a `dotnet new maui` project on a Pixel 5:

    Before:
    Average(ms): 568
    Std Err(ms): 2.21610268514595
    Std Dev(ms): 7.00793201387621
    After:
    Average(ms): 548.5
    Std Err(ms): 2.65518360947035
    Std Dev(ms): 8.39642781187333

Most notably calls to System.Reflection.Emit are now gone, after we
landed:

dotnet/android#7972
@jonathanpeppers jonathanpeppers mentioned this pull request Aug 29, 2023
Merged
@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.
Reviewers

@dellis1972 dellis1972 Awaiting requested review from dellis1972 dellis1972 is a code owner

@jonpryor jonpryor Awaiting requested review from jonpryor jonpryor is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[android] .NET 8 performance regression in System.Reflection.MethodInfo/ConstructorInfo.Invoke()
3 participants
@jonathanpeppers @pjcollins @jonpryor