-
Notifications
You must be signed in to change notification settings - Fork 527
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
Conversation
Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
* main: Bump to xamarin/Java.Interop/main@78d5937 (#8935) [docs] Add "Getting Started" docs (#8934) [Xamarin.Android.Build.Tests] Fix ActionBarSherlock URL (#8926) Update README (#8913) Bumps to xamarin/Java.Interop/main@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: [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
TODO:
|
When I went to create a standalone repro for |
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.
OK, I figured it out. The reason it broke before was that the
This results in the The solution is to update the One problem with that is that the path to the stamp file's directory is |
* main: [s360] Ignore irrelevant lz4+Python warnings (#8962) LEGO: Merge pull request 8971
...or else it breaks `AndroidManifest.xml` merging in some test cases
src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.Aot.targets
Outdated
Show resolved
Hide resolved
ENTITIES dummy!
@@ -5,28 +5,28 @@ | |||
"Size": 3036 | |||
}, | |||
"classes.dex": { | |||
"Size": 377764 | |||
"Size": 19476 |
There was a problem hiding this comment.
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?
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. |
…#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.
* main: [ci] Update .NET version installed for MAUI tests (#8995) [ci] Run "Push Internal" job on AzurePipelines-EO pool (#8991) [Xamarin.Android.Build.Tasks] LLVM Marshal Methods by Default, Take 2 (#8925) $(AndroidPackVersionSuffix)=preview.6; net9 is 34.99.0.preview.6 (#8989) [ci] Update path for generated `*.include.*` files (#8980) Bump to xamarin/xamarin-android-tools/main@da2c33e (#8984) Bump to xamarin/Java.Interop/main@f935001 (#8985) Bump to xamarin/monodroid@93ab95e18 (#8959) Force loc task pool image to windows (#8983) [git] Re-format `.gitmodules` (#8978)
No description provided.