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

Enable marshal methods by default, except when Blazor is used #8925

Merged
merged 21 commits into from
May 28, 2024

Conversation

grendello
Copy link
Member

No description provided.

grendello and others added 12 commits May 7, 2024 00:32
Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
* main:
  Bump to xamarin/java.interop@78d5937 (#8935)
  [docs] Add "Getting Started" docs (#8934)
  [Xamarin.Android.Build.Tests] Fix ActionBarSherlock URL (#8926)
  Update README (#8913)
  Bumps to xamarin/java.interop@4e893bf (#8924)
  Bump to dotnet/installer@fa261b952d 9.0.100-preview.5.24253.16 (#8921)
  [Mono.Android] Dispose of the `RunnableImplementor` on error (#8907)
  Bump NDK to r26d (#8868)
* main:
  [trimming] fix custom applications for `TrimMode=full` (#8936)
  It's ".NET for Android", not ".NET Android" (#8933)
* main:
  [build] bump to `$(AndroidNetPreviousVersion)` 34.0.113 (#8943)
* main:
  [ci] Improve maestro artifact publishing (#8945)
* main:
  [Mono.Android] AndroidMessageHandler should follow HTTP-308 redirects (#8951)
  [Microsoft.Android.Templates] Add icons to templates (#8883)
  [native] Native call tracing infra + native build system overhaul (#8857)
  [build] fix code-flow from dotnet/installer, .NET 9.0.100-preview.5.24262.2 (#8949)
  [ci] Re-enable to push to dotnet9 feed (#8950)
  LEGO: Merge pull request 8952
* main:
  Localized file check-in by OneLocBuild Task (#8957)
  LEGO: Merge pull request 8958
@jonpryor
Copy link
Member

TODO:

  • Create bug against <ILStrip/> task for it removing types when it should only be removing methods.
  • What happens on .NET 8 when $(AndroidEnableMarshalMethods)=true and $(AndroidStripILAfterAOT)=true? Does the IL stripper also remove types like Activity?

@grendello
Copy link
Member Author

When I went to create a standalone repro for ILStrip removing the type today, I found out that I can no longer reproduce it. The test still fails but the Android.App.Activity type is consistently there, while last week it was consistently not there. Right now, what's missing is the Android.App.Activity.OnCreate connector method (GetOnCreate_Landroid_os_Bundle_Handler), which causes the marshal methods classifier decide that the type has to be registered dynamically. However, since the connector method is not there, we get the exception at run time. It still looks like a problem with ILStrip, but of a different kind. I don't understand what's changed between last week and today - I use the same version of dotnet, the results should therefore be the same...

* main:
  Localized file check-in by OneLocBuild Task (#8963)
  [ci] Use long version for maestro publishing (#8964)
@grendello grendello added the do-not-merge PR should not be merged. label May 21, 2024
@grendello
Copy link
Member Author

let me do some more investigation, maybe I'll find a way to work around this test failure

The reason it broke before was that the
`EnableAndroidStripILAfterAOTFalse` runs in two steps - first it builds
the app, then it installs it (using the `Install` target).  During
the **build**, marshal methods classifier works as it should and,
consequently, marshal methods rewriter modifies the assemblies - removes
connector methods, generates wrapper methods etc etc.

`_GenerateJavaStubs`, the target which invokes marshal methods processing,
eventually creates a stamp file it then uses to decide whether all the
inputs are up to date with regards to their corresponding outputs.
However, during the **build** phase, at the end, `ILStrip` runs which
modifies assemblies **after** marshal methods processing is done.

This results in the `Install` target invoking the `_GenerateJavaStubs`
target again and MSBuild notices that the stamp file is older than some
of the inputs - assemblies modified by `ILStrip` after
`_GenerateJavaStubs` created the stamp file.  This causes the target to
run completely and, in effect, the whole marshal methods processing is
initiated again - but this time the connector methods and anything else
the classifier looks for aren't there, because they were previously
removed by the rewriter during the **build** phase.  This, in turn,
causes the classifier to decide that types need to be registered
dynamically, but they can't because the connector methods which are
required to create delegates are no longer there and we get a runtime
crash instead.

The solution is to update the `_GenerateJavaStubs` stamp file after
`ILStrip` is done, if it modified any assemblies.

One problem with that is that the path to the stamp file's directory is
different when `_GenerateJavaStubs` runs during the build phase, and
when AOT and `ILStrip` run.  In the former case, the stamp file is
created in `obj/${Configuration}/stamp` directory, in the latter case in
`obj/${Configuration}/android-ARCH/stamp` directory but **both**
locations are initialized in the same spot - at the top of
`Xamarin.Android.Common.targets` file.  In effect, after `ILStrip` runs,
we don't really know where `_GenerateJavaStubs` created its stamp file.
The workaround involves taking advantage of the fact that we know where
the stamp directories are in relation to each other.  It's a hack, but
if the relation is somehow broken, the
`EnableAndroidStripILAfterAOTFalse` test will break again.
@grendello
Copy link
Member Author

OK, I figured it out. The reason it broke before was that the
EnableAndroidStripILAfterAOTFalse runs in two steps - first it builds
the app, then it installs it (using the Install target). During
the build, marshal methods classifier works as it should and,
consequently, marshal methods rewriter modifies the assemblies - removes
connector methods, generates wrapper methods etc etc.

_GenerateJavaStubs, the target which invokes marshal methods processing,
eventually creates a stamp file it then uses to decide whether all the
inputs are up to date with regards to their corresponding outputs.
However, during the build phase, at the end, ILStrip runs which
modifies assemblies after marshal methods processing is done.

This results in the Install target invoking the _GenerateJavaStubs
target again and MSBuild notices that the stamp file is older than some
of the inputs - assemblies modified by ILStrip after
_GenerateJavaStubs created the stamp file. This causes the target to
run completely and, in effect, the whole marshal methods processing is
initiated again - but this time the connector methods and anything else
the classifier looks for aren't there, because they were previously
removed by the rewriter during the build phase. This, in turn,
causes the classifier to decide that types need to be registered
dynamically, but they can't because the connector methods which are
required to create delegates are no longer there and we get a runtime
crash instead.

The solution is to update the _GenerateJavaStubs stamp file after
ILStrip is done, if it modified any assemblies.

One problem with that is that the path to the stamp file's directory is
different when _GenerateJavaStubs runs during the build phase, and
when AOT and ILStrip run. In the former case, the stamp file is
created in obj/${Configuration}/stamp directory, in the latter case in
obj/${Configuration}/android-ARCH/stamp directory but both
locations are initialized in the same spot - at the top of
Xamarin.Android.Common.targets file. In effect, after ILStrip runs,
we don't really know where _GenerateJavaStubs created its stamp file.
The workaround involves taking advantage of the fact that we know where
the stamp directories are in relation to each other. It's a hack, but
if the relation is somehow broken, the
EnableAndroidStripILAfterAOTFalse test will break again.

@grendello grendello removed the do-not-merge PR should not be merged. label May 21, 2024
* main:
  [s360] Ignore irrelevant lz4+Python warnings (#8962)
  LEGO: Merge pull request 8971
...or else it breaks `AndroidManifest.xml` merging in some test cases
@@ -5,28 +5,28 @@
"Size": 3036
},
"classes.dex": {
"Size": 377764
"Size": 19476
Copy link
Member

Choose a reason for hiding this comment

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

wait, what? Why would enabling marshal methods impact classes.dex?! I find this deeply concerning. What types are missing due to this change, and why?

@jonpryor
Copy link
Member

jonpryor commented May 28, 2024

Draft commit message:

[Xamarin.Android.Build.Tasks] LLVM Marshal Methods by Default, Take 2 (#8925)

Context: 68368189d67c46ddbfed4e90e622f635c4aff11e
Context: 8bc7a3e84f95e70fe12790ac31ecd97957771cb2

Commit 8bc7a3e8 enabled LLVM Marshal Methods by default, and commit
68368189 *disabled* LLVM Marshal Methods by default, because it
appeared to contribute to hangs in MAUI+Blazor apps.

After lots of additional cleanup, refactoring, and investigation… we
*still* don't understand why MAUI+Blazor apps hang, and we want the
app time startup improvements that LLVM Marshal Methods allow.

Square this circle by enabling LLVM Marshal Methods by default,
*unless* Blazor is detected, in which case LLVM Marshal Methods will
be *disabled* automatically.

~~ App startup time improvements ~~

Measurements are across 50 runs if a test app, with the fastest and
slowest runs removed from the set.

  * Displayed time
      * Best result: **15.73%** faster (default settings
        *without* ProfiledAOT and compression)
      * Default settings result: **12.66%** faster.
  * Native to managed transition
      * Best result: **1.31%** faster
      * Default settings result: **0.35%** slower
  * Total managed runtime init
      * Best result: **2.55%** faster (default settings without ProfiledAOT)
      * Default settings result: **0.71%** faster

~~ Build Target Interdependencies ~~

While developing this change, we ran into an odd bug, via the
`InstallAndRunTests.EnableAndroidStripILAfterAOT(false)` test:

	% dotnet new android
	% dotnet build -c Release -v:diag -p:AndroidEnableMarshalMethods=true \
	  -p:AndroidStripILAfterAOT=true -p:AndroidEnableProfiledAot=false > b.txt
	% dotnet build -c Release -v:diag -p:AndroidEnableMarshalMethods=true \
	  -p:AndroidStripILAfterAOT=true -p:AndroidEnableProfiledAot=false -t:Install > i.txt
	% dotnet build -c Release -t:StartAndroidActivity

The app immediately crashes on startup; from `adb logcat`:

	F mono-rt : [ERROR] FATAL UNHANDLED EXCEPTION: System.InvalidProgramException: Invalid IL code in Android.Runtime.JNIEnvInit:Initialize (Android.Runtime.JNIEnvInit/JnienvInitializeArgs*): method body is empty.

The reason fo the crash is that during the first `dotnet build`
invocation, the marshal methods classifier works as it should and,
consequently, the marshal methods rewriter *modifies the assemblies*:
removes connector methods, generates wrapper methods, etc etc.
As part of this, the `_GenerateJavaStubs` target eventually creates a
stamp file which it then uses to decide whether all the inputs are up
to date with regards to their corresponding outputs.

However, at the end of the first `dotnet build`, `ILStrip` runs which
modifies assemblies *after* marshal methods processing is done.

When we run the `dotnet build -t:Install` command, the
the `_GenerateJavaStubs` target runs *again* as MSBuild notices that
the stamp file is older than some of the inputs: assemblies modified
by `ILStrip` after `_GenerateJavaStubs` created its stamp file.
This causes the target to run completely:

	Target "_GenerateJavaStubs: (TargetId:337)" in file "/usr/local/share/dotnet/packs/Microsoft.Android.Sdk.Darwin/34.0.95/tools/Xamarin.Android.Common.targets" from project "…/gxa-8925.csproj" (target "_GeneratePackageManagerJava" depends on it):
	Building target "_GenerateJavaStubs" completely.
	Input file "obj/Release/net8.0-android/android-arm/linked/gxa-8925.dll" is newer than output file "obj/Release/net8.0-android/stamp/_GenerateJavaStubs.stamp".

which causes the whole marshal methods processing to be initiated
again, but this time the connector methods and anything else the
classifier looks for aren't there, because they were previously
removed by the rewriter during the first `dotnet build` command.
This, in turn, causes the classifier to decide that types need to be
registered dynamically, but they can't because the connector methods
which are required to create delegates are no longer there and we get
a runtime crash instead.

The solution is to update the `_GenerateJavaStubs` stamp file after
`ILStrip` is done, within the `_AndroidAotCompilation` target,
if it modified any assemblies.

One problem with that is that the path to the stamp file's directory
is different when `_GenerateJavaStubs` runs during the build phase,
and when AOT and `ILStrip` run.  In the former case, the stamp file
is created in `obj/${Configuration}/stamp` directory, in the latter
case in `obj/${Configuration}/android-ARCH/stamp` directory, but
*both* locations are initialized in the same spot: at the top of
`Xamarin.Android.Common.targets`.  In effect, after `ILStrip` runs,
we don't really know where `_GenerateJavaStubs` created its stamp file.

Workaround this by taking advantage of the fact that we know where
the stamp directories are in relation to each other.  It's a hack,
but if the relation is somehow broken, the
`InstallAndRunTests.EnableAndroidStripILAfterAOT(false)` test will
break again.

@jonpryor jonpryor merged commit a9706b6 into main May 28, 2024
48 checks passed
@jonpryor jonpryor deleted the dev/grendel/enable-marshal-methods branch May 28, 2024 23:26
jonpryor pushed a commit that referenced this pull request May 30, 2024
…#8925)

Context: 6836818
Context: 8bc7a3e

Commit 8bc7a3e enabled LLVM Marshal Methods by default, and commit
6836818 *disabled* LLVM Marshal Methods by default, because it
appeared to contribute to hangs in MAUI+Blazor apps.

After lots of additional cleanup, refactoring, and investigation… we
*still* don't understand why MAUI+Blazor apps hang, and we want the
app time startup improvements that LLVM Marshal Methods allow.

Square this circle by enabling LLVM Marshal Methods by default,
*unless* Blazor is detected, in which case LLVM Marshal Methods will
be *disabled* automatically.

~~ App startup time improvements ~~

Measurements are across 50 runs if a test app, with the fastest and
slowest runs removed from the set.

  * Displayed time
      * Best result: **15.73%** faster (default settings
        *without* ProfiledAOT and compression)
      * Default settings result: **12.66%** faster.
  * Native to managed transition
      * Best result: **1.31%** faster
      * Default settings result: **0.35%** slower
  * Total managed runtime init
      * Best result: **2.55%** faster (default settings without ProfiledAOT)
      * Default settings result: **0.71%** faster


~~ Build Target Interdependencies ~~

While developing this change, we ran into an odd bug, via the
`InstallAndRunTests.EnableAndroidStripILAfterAOT(false)` test:

	% dotnet new android
	% dotnet build -c Release -v:diag -p:AndroidEnableMarshalMethods=true \
	  -p:AndroidStripILAfterAOT=true -p:AndroidEnableProfiledAot=false > b.txt
	% dotnet build -c Release -v:diag -p:AndroidEnableMarshalMethods=true \
	  -p:AndroidStripILAfterAOT=true -p:AndroidEnableProfiledAot=false -t:Install > i.txt
	% dotnet build -c Release -t:StartAndroidActivity

The app immediately crashes on startup; from `adb logcat`:

	F mono-rt : [ERROR] FATAL UNHANDLED EXCEPTION: System.InvalidProgramException: Invalid IL code in Android.Runtime.JNIEnvInit:Initialize (Android.Runtime.JNIEnvInit/JnienvInitializeArgs*): method body is empty.

The reason fo the crash is that during the first `dotnet build`
invocation, the marshal methods classifier works as it should and,
consequently, the marshal methods rewriter *modifies the assemblies*:
removes connector methods, generates wrapper methods, etc etc.
As part of this, the `_GenerateJavaStubs` target eventually creates a
stamp file which it then uses to decide whether all the inputs are up
to date with regards to their corresponding outputs.

However, at the end of the first `dotnet build`, `ILStrip` runs which
modifies assemblies *after* marshal methods processing is done.

When we run the `dotnet build -t:Install` command, the
the `_GenerateJavaStubs` target runs *again* as MSBuild notices that
the stamp file is older than some of the inputs: assemblies modified
by `ILStrip` after `_GenerateJavaStubs` created its stamp file.
This causes the target to run completely:

	Target "_GenerateJavaStubs: (TargetId:337)" in file "/usr/local/share/dotnet/packs/Microsoft.Android.Sdk.Darwin/34.0.95/tools/Xamarin.Android.Common.targets" from project "…/gxa-8925.csproj" (target "_GeneratePackageManagerJava" depends on it):
	Building target "_GenerateJavaStubs" completely.
	Input file "obj/Release/net8.0-android/android-arm/linked/gxa-8925.dll" is newer than output file "obj/Release/net8.0-android/stamp/_GenerateJavaStubs.stamp".

which causes the whole marshal methods processing to be initiated
again, but this time the connector methods and anything else the
classifier looks for aren't there, because they were previously
removed by the rewriter during the first `dotnet build` command.
This, in turn, causes the classifier to decide that types need to be
registered dynamically, but they can't because the connector methods
which are required to create delegates are no longer there and we get
a runtime crash instead.

The solution is to update the `_GenerateJavaStubs` stamp file after
`ILStrip` is done, within the `_AndroidAotCompilation` target,
if it modified any assemblies.

One problem with that is that the path to the stamp file's directory
is different when `_GenerateJavaStubs` runs during the build phase,
and when AOT and `ILStrip` run.  In the former case, the stamp file
is created in `obj/${Configuration}/stamp` directory, in the latter
case in `obj/${Configuration}/android-ARCH/stamp` directory, but
*both* locations are initialized in the same spot: at the top of
`Xamarin.Android.Common.targets`.  In effect, after `ILStrip` runs,
we don't really know where `_GenerateJavaStubs` created its stamp file.

Workaround this by taking advantage of the fact that we know where
the stamp directories are in relation to each other.  It's a hack,
but if the relation is somehow broken, the
`InstallAndRunTests.EnableAndroidStripILAfterAOT(false)` test will
break again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants