[runtime] Optionally preload all the assemblies from the apk #2724
Conversation
This file contains 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
grendello
requested review from
dellis1972,
jonathanpeppers and
jonpryor
as code owners
grendello
force-pushed
the
assembly-preload
branch
2 times, most recently
from
ddbabb6
to
3d7983d
Compare
jonpryor
added
the
full-mono-integration-build
|
build |
2 similar comments
|
build |
|
build |
|
It's very odd that the "build" comments aren't actually building the macOS PR Debug Build job. I've managed to manually kick off a job. We need the Debug results to demonstrate that the fix actually works, because the Release config will have We thus need a Debug config build to demonstrate that the fix actually fixes things. |
|
build |
jonpryor
reviewed
Feb 12, 2019
| @@ -2386,8 +2389,23 @@ because xbuild doesn't support framework reference assemblies. | |||
| </GetAddOnPlatformLibraries> | |||
| </Target> | |||
|
|
|||
| <Target Name="_SetupAssemblyPreload" | |||
| Condition=" '$(AndroidEnablePreloadAssemblies)' == 'True' " | |||
| Inputs="$(IntermediateOutputPath)javastubs.cache" | |||
There was a problem hiding this comment.
I don't immediately understand why this would use $(IntermediateOutputPath)javastubs.cache as an input...though that is what _SetupEmbeddedDSOs does...
There was a problem hiding this comment.
I don't think an appropriate Inputs/Outputs exists for this target, since there isn't a file that would influence what this does. I think we could just use <WriteLinesToFile WriteOnlyWhenDifferent="True"/> and not use any Inputs/Outputs. msbuild docs
_GenerateJavaStubs writes javastubs.cache looking at android:extractNativeLibs="false", which determines the value of $(_EmbeddedDSOsEnabled). So that makes sense for _SetupEmbeddedDSOs to use javastubs.cache as an input.
jonpryor
reviewed
Feb 12, 2019
jonpryor
reviewed
Feb 12, 2019
| @@ -779,6 +779,11 @@ AndroidSystem::setup_environment (jstring_wrapper& name, jstring_wrapper& value) | |||
| knownEnvVars.MonoLLVM = true; | |||
| return; | |||
| } | |||
|
|
|||
| if (strcmp (k, "mono.enable_assembly_preload") == 0) { | |||
There was a problem hiding this comment.
...and regardless of what the default is, this should actually use the value specified. If someone does:
mono.enable_assembly_preload=0
i.e. explicitly disable it via a user-provided @(AndroidEnvironment) file, then it should be disabled.
[Startup performance][0] commit dispensed with preloading of all assemblies from the APK since it's no longer needed by Xamarin.Android, thus improving the overall startup time. However, it turns out that some applications (especially those using Xamarin.Forms, but not only) rely on the fact that all the assemblies are loaded by the time the application code is called. They do it by calling `AppDomain.CurrentDomain.GetAssemblies()` and iterating over the returned array in order to load resources, inject dependencies etc. This is not a correct way to do it and nobody should rely on the `GetAssemblies()` call returning all the assemblies available, but since this is currently required by the aforementioned applications, we need to hurt performance and restore the preload behavior albeit this time making it optional. This commit implements the preload in a similar way as before, however this time it is controlled by MSBuild property `$(AndroidEnablePreloadAssemblies)` (which currently defaults to `true`) which the developer can set in their project file and thus control the runtime behavior. The commit also updates (courtesy of Jon Peppers @jonathanpeppers) the Xamarin.Forms performance integration app in Xamarin.Android tests to fail in case the assemblies are *not* preloaded. Currently, this application yields the following performance hit on Pixel 3 XL with the preload enabled: 02-11 21:19:58.647 27410 27410 I monodroid-timing: Assembly load: FormsViewGroup.dll preloaded; elapsed: 0s:1::267136 02-11 21:19:58.648 27410 27410 I monodroid-timing: Assembly load: Newtonsoft.Json.dll preloaded; elapsed: 0s:0::999584 02-11 21:19:58.649 27410 27410 I monodroid-timing: Assembly load: Plugin.Connectivity.Abstractions.dll preloaded; elapsed: 0s:0::916459 02-11 21:19:58.650 27410 27410 I monodroid-timing: Assembly load: Plugin.Connectivity.dll preloaded; elapsed: 0s:0::886146 02-11 21:19:58.651 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Arch.Core.Common.dll preloaded; elapsed: 0s:0::890208 02-11 21:19:58.652 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Arch.Lifecycle.Common.dll preloaded; elapsed: 0s:0::918229 02-11 21:19:58.653 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Arch.Lifecycle.Runtime.dll preloaded; elapsed: 0s:0::880833 02-11 21:19:58.653 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.Animated.Vector.Drawable.dll preloaded; elapsed: 0s:0::894323 02-11 21:19:58.654 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.Annotations.dll preloaded; elapsed: 0s:0::928177 02-11 21:19:58.655 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.Compat.dll preloaded; elapsed: 0s:0::941719 02-11 21:19:58.656 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.Core.UI.dll preloaded; elapsed: 0s:0::936563 02-11 21:19:58.657 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.Core.Utils.dll preloaded; elapsed: 0s:0::938594 02-11 21:19:58.658 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.Design.dll preloaded; elapsed: 0s:0::983334 02-11 21:19:58.659 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.Fragment.dll preloaded; elapsed: 0s:0::946042 02-11 21:19:58.660 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.Media.Compat.dll preloaded; elapsed: 0s:0::927604 02-11 21:19:58.661 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.Transition.dll preloaded; elapsed: 0s:0::959376 02-11 21:19:58.662 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.v4.dll preloaded; elapsed: 0s:0::923750 02-11 21:19:58.663 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.v7.AppCompat.dll preloaded; elapsed: 0s:1::82500 02-11 21:19:58.664 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.v7.CardView.dll preloaded; elapsed: 0s:0::920313 02-11 21:19:58.665 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.v7.MediaRouter.dll preloaded; elapsed: 0s:0::949323 02-11 21:19:58.666 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.v7.Palette.dll preloaded; elapsed: 0s:0::906093 02-11 21:19:58.667 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.v7.RecyclerView.dll preloaded; elapsed: 0s:0::900782 02-11 21:19:58.668 27410 27410 I monodroid-timing: Assembly load: Xamarin.Android.Support.Vector.Drawable.dll preloaded; elapsed: 0s:0::960416 02-11 21:19:58.669 27410 27410 I monodroid-timing: Assembly load: Xamarin.Forms.Core.dll preloaded; elapsed: 0s:0::972396 02-11 21:19:58.670 27410 27410 I monodroid-timing: Assembly load: Xamarin.Forms.Performance.Integration.dll preloaded; elapsed: 0s:0::898646 02-11 21:19:58.671 27410 27410 I monodroid-timing: Assembly load: Xamarin.Forms.Platform.Android.dll preloaded; elapsed: 0s:0::932813 02-11 21:19:58.672 27410 27410 I monodroid-timing: Assembly load: Xamarin.Forms.Platform.dll preloaded; elapsed: 0s:0::935677 02-11 21:19:58.673 27410 27410 I monodroid-timing: Assembly load: Xamarin.Forms.Xaml.dll preloaded; elapsed: 0s:0::928594 02-11 21:19:58.673 27410 27410 I monodroid-timing: Assembly load: preloaded 29 assemblies; elapsed: 0s:26::783805 [0]: b90d3ab
grendello
force-pushed
the
assembly-preload
branch
from
1a258ff
to
e230162
Compare
dellis1972
reviewed
Feb 12, 2019
| static void | ||
| monodroid_Mono_UnhandledException_internal (MonoException *ex) | ||
| { | ||
| // Do nothing with it here, we let the exception naturally propagate on the managed side | ||
| } | ||
|
|
||
| static MonoDomain* | ||
| create_and_initialize_domain (JNIEnv* env, jclass runtimeClass, jstring_array_wrapper &runtimeApks, jobjectArray assemblies, jobject loader, bool is_root_domain) | ||
| create_and_initialize_domain (JNIEnv* env, jclass runtimeClass, jstring_array_wrapper &runtimeApks, jstring_array_wrapper &assemblies, jobject loader, bool is_root_domain) |
There was a problem hiding this comment.
@grendello I take it this means we can't get rid of the list of assemblies being passed in from the Java side now?
There was a problem hiding this comment.
@dellis1972: that's exactly what this means
|
We can ignore the Windows test failure: I have not seen this happen that often since adding |
jonpryor
pushed a commit
that referenced
this pull request
Feb 12, 2019
Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/789540 Context: xamarin/Xamarin.Forms#5164 Commit b90d3ab altered how assemblies were loaded during process startup in an effort to reduce time spent in `Runtime.init()`: assembly loading has unavoidable overhead. Reducing the number of assemblies loaded results in less time spent in `Runtime.init()`. This change also meant that the semantics of `System.AppDomain.GetAssemblies()` changed: prior to b90d3ab, `AppDomain.CurrentDomain.GetAssemblies()` would return *all* assemblies present within the `.apk`. Starting with b90d3ab, `AppDomain.CurrentDomain.GetAssemblies()` would *instead* return only the assemblies which had been loaded *up to that point in time*, which may not be "everything". We honestly didn't spend too much time thinking about this particular change. The b90d3ab behavior is what desktop .NET has done since the beginning -- which makes sense, as desktop .NET didn't have a concept of "application" for which one could ask for "all assemblies that make up the application" -- so why would this break anything? Anything broken in such an environment would fail on .NET. Turns Out™ that there are mobile frameworks which assume that `AppDomain.CurrentDomain.GetAssemblies()` will return all known assemblies, and thus *break* with b90d3ab, including: * [Xamarin.Forms][0] * [MvvmCross][1] Which brings us to [Xamarin.Forms Issue #5164][2], in which a Xamarin.Forms app which uses CSS crashes with an NRE: UNHANDLED EXCEPTION: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.NullReferenceException: Object reference not set to an instance of an object. at Xamarin.Forms.Xaml.ApplyPropertiesVisitor.ProvideValue (System.Object& value, Xamarin.Forms.Xaml.ElementNode node, System.Object source, Xamarin.Forms.Xaml.XmlName propertyName) [0x000a7] in D:\a\1\s\Xamarin.Forms.Xaml\ApplyPropertiesVisitor.cs:243 at Xamarin.Forms.Xaml.ApplyPropertiesVisitor.Visit (Xamarin.Forms.Xaml.ElementNode node, Xamarin.Forms.Xaml.INode parentNode) [0x000c9] in D:\a\1\s\Xamarin.Forms.Xaml\ApplyPropertiesVisitor.cs:107 at Xamarin.Forms.Xaml.ElementNode.Accept (Xamarin.Forms.Xaml.IXamlNodeVisitor visitor, Xamarin.Forms.Xaml.INode parentNode) [0x000ac] in D:\a\1\s\Xamarin.Forms.Xaml\XamlNode.cs:149 at Xamarin.Forms.Xaml.RootNode.Accept (Xamarin.Forms.Xaml.IXamlNodeVisitor visitor, Xamarin.Forms.Xaml.INode parentNode) [0x00044] in D:\a\1\s\Xamarin.Forms.Xaml\XamlNode.cs:200 at Xamarin.Forms.Xaml.XamlLoader.Visit (Xamarin.Forms.Xaml.RootNode rootnode, Xamarin.Forms.Xaml.HydrationContext visitorContext, System.Boolean useDesignProperties) [0x0008b] in D:\a\1\s\Xamarin.Forms.Xaml\XamlLoader.cs:150 at Xamarin.Forms.Xaml.XamlLoader.Load (System.Object view, System.String xaml, System.Boolean useDesignProperties) [0x00058] in D:\a\1\s\Xamarin.Forms.Xaml\XamlLoader.cs:91 at Xamarin.Forms.Xaml.XamlLoader.Load (System.Object view, System.Type callingType) [0x00028] in D:\a\1\s\Xamarin.Forms.Xaml\XamlLoader.cs:69 at Xamarin.Forms.Xaml.Extensions.LoadFromXaml[TXaml] (TXaml view, System.Type callingType) [0x00000] in D:\a\1\s\Xamarin.Forms.Xaml\ViewExtensions.cs:36 at TheLittleThingsPlayground.Views.ThreeFivePage.InitializeComponent () [0x00001] in H:\github\TheLittleThingsPlayground\TheLittleThingsPlayground\obj\Debug g.cs:21 at TheLittleThingsPlayground.Views.ThreeFivePage..ctor () [0x00008] in H:\github\TheLittleThingsPlayground\TheLittleThingsPlayground\Views\ThreeFivePage.xaml.cs:12 at (wrapper managed-to-native) System.Reflection.MonoCMethod.InternalInvoke(System.Reflection.MonoCMethod,object,object[],System.Exception&) at System.Reflection.MonoCMethod.InternalInvoke (System.Object obj, System.Object[] parameters, System.Boolean wrapExceptions) [0x00005] in <84c6975c2cbc47b489a2a76477d7a312>:0 The `Xamarin.Forms.Xaml.dll` assembly isn't loaded when the `ThreeFivePage` type is constructed, which eventually means that a call to `DependencyService.Get<IResourcesLoader>()` returns `null`, because `Xamarin.Forms.Xaml.ResourcesLoader, Xamarin.Forms.Xaml` was not found or registered as an implementation for `IResourcesLoader`. There is a *workaround* for a post-b90d3ab7 environment: explicitly load the required assembly/assemblies earlier in the process: // In your MainActivity.cs protected override void OnCreate(Bundle bundle) { // Workaround Assembly.Load("Xamarin.Forms.Xaml"); base.OnCreate(bundle); global::Xamarin.Forms.Forms.Init(this, bundle); LoadApplication(new App()); } However, while there may be a workaround, this semantic change has unknown impact on the overall ecosystem and environment. Apps WILL break because of this change. We could revert the assembly loading portion of b90d3ab, but that means *all* apps need to pay the cost of loading all assemblies, even if they don't need to pay that startup cost, a cost of ~30ms for the `tests/Xamarin.Forms-Performance-Integration` app. Instead, we'll enable the new functionality to be "opt-in", and make the previous behavior the default. Add a new `$(AndroidEnablePreloadAssemblies)` MSBuild property which, when True, restores the previous behavior of loading all assemblies within the `.apk` during process startup. The default value is True. If the `debug.mono.log` system property is set and contains `timing`, `adb logcat` will contain messages regarding how much time was spent loading each assembly, and the sum total for all assemblies: monodroid-timing: Assembly load: FormsViewGroup.dll preloaded; elapsed: 0s:1::267136 monodroid-timing: Assembly load: Newtonsoft.Json.dll preloaded; elapsed: 0s:0::999584 ... monodroid-timing: Finished loading assemblies: preloaded 29 assemblies; elapsed: 0s:26::783805 Finally, update `tests/Xamarin.Forms-Performance-Integration` so that it uses CSS, and thus hits the Xamarin.Forms codepath that triggers the above `NullReferenceException. This should validate that the fix is correct, and help prevent future regressions. [0]: https://github.com/xamarin/Xamarin.Forms/blob/cd1cf19fcad6ae049e90fdc5819c4f7c6adb57f6/Xamarin.Forms.Platform.Android/Forms.cs#L417 [1]: https://github.com/MvvmCross/MvvmCross/blob/43c1c7848d482adbc2bdc984746f36d74f64c190/MvvmCross/Core/MvxSetup.cs#L34 [2]: xamarin/Xamarin.Forms#5164
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Startup performance commit dispensed with preloading of all assemblies from
the APK since it's no longer needed by Xamarin.Android, thus improving the
overall startup time. However, it turns out that some applications (especially
those using Xamarin.Forms, but not only) rely on the fact that all the
assemblies are loaded by the time the application code is called. They do it by
calling
AppDomain.CurrentDomain.GetAssemblies()and iterating over thereturned array in order to load resources, inject dependencies etc. This is not
a correct way to do it and nobody should rely on the
GetAssemblies()callreturning all the assemblies available, but since this is currently required by
the aforementioned applications, we need to hurt performance and restore the
preload behavior albeit this time making it optional.
This commit implements the preload in a similar way as before, however this time
it is controlled by MSBuild property
$(AndroidEnablePreloadAssemblies)(which currently defaults to
true) which the developer can set in theirproject file and thus control the runtime behavior.
The commit also updates (courtesy of Jon Peppers @jonathanpeppers) the
Xamarin.Forms performance integration app in Xamarin.Android tests to fail in
case the assemblies are not preloaded. Currently, this application yields the
following performance hit on Pixel 3 XL with the preload enabled: