Skip to content

Commit

Permalink
[Xamarin.Android.Build.Tasks] LLVM Marshal Methods by Default, Take 2 (
Browse files Browse the repository at this point in the history
…#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.
  • Loading branch information
grendello authored and jonpryor committed May 30, 2024
1 parent f1284d6 commit c8158ac
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 73 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,30 @@ They run in a context of an inner build with a single $(RuntimeIdentifier).
SourceFiles="@(_ILStripUpdatedAssemblies)"
DestinationFiles="@(_ILStripUpdatedAssemblies->'%(UntrimmedAssemblyFilePath)')"
/>
<ItemGroup>
<_UpdateStamp Include="@(_ILStripUpdatedAssemblies)" Condition=" '$(AndroidStripILAfterAOT)' == 'true' and '%(_ILStripUpdatedAssemblies.ILStripped)' == 'true' " />
</ItemGroup>

<!-- We must update the stamp file used by `_GenerateJavaStubs`, but `$(_AndroidStampDirectory)` **here** is inside a per-RID parent
directory (e.g. `obj/Release/android-arm64/stamp`) and not the "top level" one (`obj/Release/stamp`). Since **both** are set
in the same place (in `Xamarin.Android.Common.targets`), we don't know where the "top level" one really is. Hence the hack.
If the locations or their relative paths change, then the `EnableAndroidStripILAfterAOTFalse` test will fail.
However, there should be a better way to do it... Ideally, when marshal methods are enabled **nothing** should modify any
assemblies after marshal method classifier and rewriter runs.
-->
<PropertyGroup>
<_StampDir>$(_AndroidStampDirectory)\..\..\stamp</_StampDir>
</PropertyGroup>
<MakeDir
Condition=" '$(AndroidStripILAfterAOT)' == 'true' and '@(_UpdateStamp->Count())' &gt; 0 "
Directories="$(_StampDir)"
/>
<Touch
Condition=" '$(AndroidStripILAfterAOT)' == 'true' and '@(_UpdateStamp->Count())' &gt; 0 "
Files="$(_StampDir)\_GenerateJavaStubs.stamp"
AlwaysCreate="True"
/>
<WriteLinesToFile
File="$(_AndroidStampDirectory)_AndroidAot.stamp"
Lines="@(_MonoAOTCompiledAssemblies->'%(LibraryFile)')"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,28 @@
"Size": 3036
},
"classes.dex": {
"Size": 377764
"Size": 19476
},
"lib/arm64-v8a/lib__Microsoft.Android.Resource.Designer.dll.so": {
"Size": 1027
},
"lib/arm64-v8a/lib_Java.Interop.dll.so": {
"Size": 64231
"Size": 64655
},
"lib/arm64-v8a/lib_Mono.Android.dll.so": {
"Size": 91590
"Size": 92217
},
"lib/arm64-v8a/lib_Mono.Android.Runtime.dll.so": {
"Size": 5315
"Size": 5320
},
"lib/arm64-v8a/lib_System.Console.dll.so": {
"Size": 6539
"Size": 6541
},
"lib/arm64-v8a/lib_System.Linq.dll.so": {
"Size": 8525
"Size": 8524
},
"lib/arm64-v8a/lib_System.Private.CoreLib.dll.so": {
"Size": 570166
"Size": 571267
},
"lib/arm64-v8a/lib_System.Runtime.dll.so": {
"Size": 2544
Expand All @@ -35,7 +35,7 @@
"Size": 4020
},
"lib/arm64-v8a/lib_UnnamedProject.dll.so": {
"Size": 2933
"Size": 2931
},
"lib/arm64-v8a/libarc.bin.so": {
"Size": 1586
Expand All @@ -44,10 +44,10 @@
"Size": 87352
},
"lib/arm64-v8a/libmonodroid.so": {
"Size": 476432
"Size": 492104
},
"lib/arm64-v8a/libmonosgen-2.0.so": {
"Size": 3138768
"Size": 3154304
},
"lib/arm64-v8a/libSystem.Globalization.Native.so": {
"Size": 67248
Expand All @@ -56,16 +56,16 @@
"Size": 723560
},
"lib/arm64-v8a/libSystem.Native.so": {
"Size": 94720
"Size": 95296
},
"lib/arm64-v8a/libSystem.Security.Cryptography.Native.Android.so": {
"Size": 155568
},
"lib/arm64-v8a/libxamarin-app.so": {
"Size": 12696
"Size": 17952
},
"META-INF/BNDLTOOL.RSA": {
"Size": 1221
"Size": 1223
},
"META-INF/BNDLTOOL.SF": {
"Size": 3266
Expand Down Expand Up @@ -98,5 +98,5 @@
"Size": 1904
}
},
"PackageSize": 2742805
"PackageSize": 2681365
}
Loading

0 comments on commit c8158ac

Please sign in to comment.