Disable assembly preload by default #5790
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Comment on lines
293
to
295
| <!-- If true it will cause all the assemblies in the apk to be preloaded on startup time --> | ||
| <_AndroidEnablePreloadAssembliesDefault>True</_AndroidEnablePreloadAssembliesDefault> | ||
| <_AndroidEnablePreloadAssembliesDefault>False</_AndroidEnablePreloadAssembliesDefault> | ||
| <AndroidEnablePreloadAssemblies Condition=" '$(AndroidEnablePreloadAssemblies)' == '' ">$(_AndroidEnablePreloadAssembliesDefault)</AndroidEnablePreloadAssemblies> |
Member
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
And drop _AndroidEnablePreloadAssembliesDefault completely?
Contributor
Author
There was a problem hiding this comment.
A couple other places use $(_AndroidEnablePreloadAssembliesDefault) so I instead added the condition to _AndroidEnablePreloadAssembliesDefault itself
grendello
force-pushed
the
disable-assembly-preload-by-default
branch
3 times, most recently
from
d834002 to
05ee6bd
Compare
jonpryor
reviewed
Mar 29, 2021
| <!-- 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> |
Contributor
There was a problem hiding this comment.
I would suggest using != 'true, which will nicely match both false and empty string and /p:UsingAndroidNETSDK=ThisIsASeriouslyWeirdValue.
grendello
force-pushed
the
disable-assembly-preload-by-default
branch
from
05ee6bd to
7f36eb3
Compare
Contributor
|
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 |
jonpryor
reviewed
Mar 30, 2021
| <!-- 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> |
Contributor
There was a problem hiding this comment.
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?
grendello
force-pushed
the
disable-assembly-preload-by-default
branch
from
7f36eb3 to
1e65355
Compare
Contributor
Author
|
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.
grendello
force-pushed
the
disable-assembly-preload-by-default
branch
from
1e65355 to
7fce2dd
Compare
jonathanpeppers
approved these changes
Mar 30, 2021
grendello
added a commit
to grendello/xamarin-android
that referenced
this pull request
May 5, 2021
…#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
jonpryor
pushed a commit
that referenced
this pull request
May 7, 2021
…#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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.