-
Notifications
You must be signed in to change notification settings - Fork 526
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
Disable assembly preload by default #5790
Disable assembly preload by default #5790
Conversation
<_AndroidEnablePreloadAssembliesDefault>True</_AndroidEnablePreloadAssembliesDefault> | ||
<_AndroidEnablePreloadAssembliesDefault>False</_AndroidEnablePreloadAssembliesDefault> | ||
<AndroidEnablePreloadAssemblies Condition=" '$(AndroidEnablePreloadAssemblies)' == '' ">$(_AndroidEnablePreloadAssembliesDefault)</AndroidEnablePreloadAssemblies> |
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.
Maybe we should only do this for .NET 6, so instead something like:
<AndroidEnablePreloadAssemblies Condition=" '$(AndroidEnablePreloadAssemblies)' == '' and '$(UsingAndroidNETSdk)' == 'true' ">false</AndroidEnablePreloadAssemblies>
<AndroidEnablePreloadAssemblies Condition=" '$(AndroidEnablePreloadAssemblies)' == '' and '$(UsingAndroidNETSdk)' != 'true' ">true</AndroidEnablePreloadAssemblies>
$(UsingAndroidNETSdk)
is true
under .NET 6.
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.
And drop _AndroidEnablePreloadAssembliesDefault
completely?
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.
A couple other places use $(_AndroidEnablePreloadAssembliesDefault)
so I instead added the condition to _AndroidEnablePreloadAssembliesDefault
itself
d834002
to
05ee6bd
Compare
@@ -290,7 +290,8 @@ Copyright (C) 2011-2012 Xamarin. All rights reserved. | |||
<AndroidMakeBundleKeepTemporaryFiles Condition=" '$(AndroidMakeBundleKeepTemporaryFiles)' == '' ">False</AndroidMakeBundleKeepTemporaryFiles> | |||
|
|||
<!-- If true it will cause all the assemblies in the apk to be preloaded on startup time --> | |||
<_AndroidEnablePreloadAssembliesDefault>True</_AndroidEnablePreloadAssembliesDefault> | |||
<_AndroidEnablePreloadAssembliesDefault Condition=" '$(UsingAndroidNETSdk)' == 'true' ">False</_AndroidEnablePreloadAssembliesDefault> | |||
<_AndroidEnablePreloadAssembliesDefault Condition=" '$(UsingAndroidNETSdk)' == 'false' Or '$(UsingAndroidNETSdk)' == '' ">True</_AndroidEnablePreloadAssembliesDefault> |
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.
I would suggest using != 'true
, which will nicely match both false
and empty string and /p:UsingAndroidNETSDK=ThisIsASeriouslyWeirdValue
.
05ee6bd
to
7f36eb3
Compare
We now have unit test failures, e.g.
Appears to be this assert: https://github.com/xamarin/xamarin-android/blob/c9e2d7540393f25d854014d158b4eb7c66a2b7c2/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/BuildTest.cs#L3935 Is |
@@ -290,7 +290,8 @@ Copyright (C) 2011-2012 Xamarin. All rights reserved. | |||
<AndroidMakeBundleKeepTemporaryFiles Condition=" '$(AndroidMakeBundleKeepTemporaryFiles)' == '' ">False</AndroidMakeBundleKeepTemporaryFiles> | |||
|
|||
<!-- If true it will cause all the assemblies in the apk to be preloaded on startup time --> | |||
<_AndroidEnablePreloadAssembliesDefault>True</_AndroidEnablePreloadAssembliesDefault> | |||
<_AndroidEnablePreloadAssembliesDefault Condition=" '$(UsingAndroidNETSdk)' == 'true' ">False</_AndroidEnablePreloadAssembliesDefault> | |||
<_AndroidEnablePreloadAssembliesDefault Condition=" '$(UsingAndroidNETSdk)' != 'true' ">True</_AndroidEnablePreloadAssembliesDefault> |
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.
Since this PR will require additional changes anyway…
Could you please indent this line to match the following line, removing some of the leading whitespace?
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.
Done
7f36eb3
to
1e65355
Compare
|
Disable the current default behavior of Xamarin.Android runtime to preload all the assemblies shipped in the apk on application startup. This behavior has been the default in order to support older versions of `Xamarin.Forms`, sacrificing startup performance.
1e65355
to
7fce2dd
Compare
…#5790) Fixes: dotnet#5838 Context: e0da1f1 Partially reverts 522d7fb which reverted d13d0f9 The [`$(AndroidEnablePreloadAssemblies)`][0] property controls whether or not *all* `.dll` files contained within a `.apk` are loaded during process startup. *Not* doing so reduces process startup times, which is desirable, but this also caused certain Xamarin.Forms apps to fail to run as they previously had, as not loading all assemblies during startup broke their Dependency Injection infrastructure. For .NET 6, we feel we have *some* "wiggle-room" to change default semantics, so for .NET 6 projects set the default value of `$(AndroidEnablePreloadAssemblies)` to False, so that assemblies are *not* pre-loaded during process startup. `$(AndroidEnablePreloadAssemblies)` can be set to True within the app's `.csproj` file to return to "legacy" semantics. This will cause all assemblies to be loaded during startup, with a commensurate increase in app startup overheads. [0]: https://docs.microsoft.com/en-us/xamarin/android/deploy-test/building-apps/build-properties#androidenablepreloadassemblies
…#5914) Fixes: #5838 Context: e0da1f1 Partially reverts 522d7fb which, reverted d13d0f9. The [`$(AndroidEnablePreloadAssemblies)`][0] property controls whether or not *all* `.dll` files contained within a `.apk` are loaded during process startup. *Not* doing so reduces process startup times, which is desirable, but this also caused certain Xamarin.Forms apps to fail to run as they previously had, as not loading all assemblies during startup broke their Dependency Injection infrastructure. For .NET 6, we feel we have *some* "wiggle-room" to change default semantics, so for .NET 6 projects set the default value of `$(AndroidEnablePreloadAssemblies)` to False, so that assemblies are *not* pre-loaded during process startup. `$(AndroidEnablePreloadAssemblies)` can be set to True within the app's `.csproj` file to return to "legacy" semantics. This will cause all assemblies to be loaded during startup, with a commensurate increase in app startup overheads. [0]: https://docs.microsoft.com/en-us/xamarin/android/deploy-test/building-apps/build-properties#androidenablepreloadassemblies
Disable the current default behavior of Xamarin.Android runtime to
preload all the assemblies shipped in the apk on application startup.
This behavior has been the default in order to support older versions of
Xamarin.Forms
, sacrificing startup performance.