Runtime startup performance improvements

[grendello]

The goal of this commit is to make Xamarin.Android apps start faster. The
changes are focused only around the very first stage of application startup -
between Android invoking our Java startup code (MonoPackageManager) and the end
of our Runtime.init (Java_mono_android_Runtime_init in the native runtime)
which is when the user application is fully initialized and ready to start is
launcher Activity.

In order to achieve the goal, the following changes were made:

• Java class lookup ("reflection").
  We used to call the FindClass JNI function as part of the startup at a cost
  of several milliseconds. We now put the class handles (accessed with the
  .class Java accessor) in the Runtime class and initialize them from the
  static constructor. We then read those fields from within Runtime.init,
  which is passed a reference to the Java instance of the Runtime class.
  Additonally, a handful of class field/method lookups were moved out of the
  init code so that the code that doesn't use them doesn't have to pay the tax.

• Android API level is passed to Runtime.init from Java instead of using JNI
  from the native code.

• Limit logging.
  Previously whenever any of the log_{info,debug} functions were called we'd
  spend time preparing all the parameters to pass to the function, sometimes
  involving memory allocation, function calls, etc - only to discard all of
  that work inside the log_* call because the logging category used in
  that call was disabled. Now we check whether the category is enabled before
  we set out to construct the parameters.

• Java/JNI type wrappers for string and array of strings.
  This both a convenience/correctness as well as a performance change.
  Introduced are two C++ wrapper classes for the jstring and object array
  types (specialized for object == jstring) which take care of efficiently
  caching the retrieved strings as well as of correctly deleting local
  references to the obtained objects. Both classes, jstring_wrapper and
  jstring_array_wrapper are optimized so that they compile into the
  equivalent of the current, hand-written, code. They also take care to make
  the minimum necessary number of calls in order to access the strings, both
  standalone and from arrays, as well as to release the resources.
  The string and array wrappers are passed around as references, thus using the
  minimum amount of memory.

• Do not preload managed assemblies.
  We used to preload all of the application assemblies in order to find and
  invoke type initializers. This resulted in the list of assemblies being
  processed twice at the great expense of time. We now don't call the type
  initializers at all and the assemblies are loaded on demand.

• Do not store application environment variables in a file inside the apk.
  The textual file used to be read from the apk(s) early in the process,
  requiring iteration over all the application apk files, opening each of them,
  browsing through the ZIP entries and, finally, reading the file line by line,
  parsing into the name and value parts to create either a
  property (mono.aot, mono.llvm) or any environment variables requested by
  the application developer (or the XA runtime).
  To speed the process up, this commit replaces the text file with a Java class
  generated during application build which contains an array of "name", "value" pairs. The class is passed to Java_mono_android_Runtime_init and
  its elements are used to create the requested environment variables. A
  handful of variables is special-cased in that they are not placed in the
  environment but rather to set flags in the AndroidSystem class. The
  variables are mono.aot, mono.llvm and __XA_DSO_IN_APK. This allowed to
  remove calls to create (fake) system properties as well as getenv in the
  init native function.

• Don't try load LLVM.so when it won't be there because we're not using llvm

• Convert package name to hash using optimized code without calling snprintf

• Desktop build is determined on compile time instead of dynamically

• xamarin_getifaddrs are initialized on demand, not at the startup.

Startup time improvements for the XF integration test app (average, Pixel 3 XL):

• Debug mode:
  Old: 1s 440ms
  New: 1s 100ms

• Release mode:
  Old: 650ms
  New: 270ms

[garuma]

Would it be possible to have an equivalent branch on monodroid? That way I can both buddy test manually and launch a CI run on the changes.

[radekdoulik]

Are we dropping that?

[grendello]

Yep, assemblies are loaded fully on-demand now

[jonathanpeppers]

@grendello if assemblies are loaded on-demand now, we should check and make sure a scenario still works in Xamarin.Forms:

1. A custom renderer (or anything using their DI) is defined in a separate assembly than the app
2. Xamarin.Forms hits AppDomain.GetAssemblies: https://github.com/xamarin/Xamarin.Forms/blob/505a855cad39322c8b235281d71c35b70c9df129/Xamarin.Forms.Platform.Android/Forms.cs#L412-L415

I'm wondering if something would break here?

I have a test project you could try: https://github.com/jonathanpeppers/glidex/tree/master/glidex.forms.sample

[grendello]

@jonathanpeppers I'll test it, thanks. It would be unfortunate of XF relied on AppDomain.GetAssemblies to work properly...

[grendello]

@jonathanpeppers just tested - it works! :)

[dellis1972]

Looks ok to me.

[jonpryor]

Is a static block actually needed, as opposed to just inline initialization?

[grendello]

Is there a difference? I thought this looked nicely delimited from the other code, just that

[jonpryor]

The difference is that when we add Yet Another Field, using "inline assignment" means the new field is a ~1 line change, not a 2 line change, and it would presumably be harder to accidentally forget the assignment line.

Otherwise, I don't think there's a huge difference.

[jonpryor]

Instead of strdup()ing a string into knownEnvVars.MonoAOT, should the string instead be mapped into a MonoAotMode enum so that we don't need to re-switch on the first character of knownEnvVars.MonoAOT? It would save memory, at least.

[jonpryor]

Why protected and not private? This type isn't currently subclassed.

We should also declare-but-not-implement the copy constructor and assignment operator:

jstring_wrapper (const jstring_wrapper&);
jstring_wrapper& operator=(const jstring_wrapper&);

These members cannot be public, as we cannot sanely permit copying of these instances.

[jonpryor]

why jstring and not const jstring?

[jonpryor]

Has exception safety changed in the last 20 years? Shouldn't this instead be akin to:

jstring_wrapper t(env, new_js);
std::swap (*this, t);

with an appropriate specialization for std::swap()?

[grendello]

We don't use exceptions and we don't want to use the STL. There's no danger here.

[grendello]

Note: we're currently using a tiny bit of STL stuff (std::atomic and std::mutex) and I also discovered that clang in the NDK does not remove RTTI or exception stuff despite being passed -fno-rtti and -fno-exceptions (gcc correctly removes everything related to RTTI and exceptions). I think, for now, the use of STL can stay until we have time to replace those features with something local. Regarding clang, I'm going to investigate that.

[grendello]

I need to figure out what's going on with RTTI and exceptions before this can be merged

[jonpryor]

Typo in the error message.

[jonpryor]

Why not just stick the environment file into the mono package, so it can just reuse OutputDirectory?

[grendello]

Because conceptually it is application environment, so it belongs in the app package and also, src/mono/android/app is created anyway so we're reusing an existing location anyway.

[jonpryor]

Wait...this is in a #if defined (WINDOWS) block.

Why's this need to be changed? Does std::mutex even work on Windows, or in our MXE environment?

/me adds full-mono-integration-build. We'll need to rebuild things. :-/

[grendello]

It's a part of the standard C++11: https://en.cppreference.com/w/cpp/thread/mutex

We build on mac/linux with gcc which is fully C++11 compliant and I imagine that the current msvc is compliant as well.

[grendello]

It seems msvc is fine with <mutex>: https://docs.microsoft.com/en-us/cpp/standard-library/mutex?view=vs-2017

[jonpryor]

I'm surprised this wasn't already in here. ¯_(ツ)_/¯

[jonpryor]

Why are we moving away from the C++ headers?

Are we now aiming for a C++ Runtime Library of none?

There is also the option to have no STL. There are no linking or licensing requirements in that case. No C++ standard headers are available.

I'm not even sure how we'd set that in CMakeLists.txt.

[grendello]

<climits> is just a wrapper around limits.h which is already included by other headers in the sources, it's just not needed. Also, climits is not part of the STL but of the C++ standard runtime.

[grendello]

As for the STL we use system (in https://github.com/grendello/xamarin-android/blob/runtime-profile/build-tools/scripts/cmake-common.props#L5) which is fine for us.

[jonpryor]

Will this value be passed into mono, and if so, what will happen?

[grendello]

It won't be passed to Mono (https://github.com/grendello/xamarin-android/blob/runtime-profile/src/monodroid/jni/monodroid-glue.cc#L1904-L1905)

[jonpryor]

Could/should this be static? I'm not sure I see much sense in "wasting" 3sizeof(void) bytes of space.

[grendello]

They should not be static. That would make them shared across all the instances - dangerous and invalid (especially for the array). This is a tradeoff between memory use (not much of it used) and speed (large savings when we don't have to allocate the wrappers array for each instance of the class). The only one out of these three that could be static is invalid_wrapper but that again creates uncertainty about what happens if the returned reference is retained and used in an unknown way by some unknown future code - a static version would be shared across all the code that would get it and any modifications to it would be visible everywhere the reference is retained. In short - it introduces uncertainty for the sake of saving two sizeof(void*) which is not worth it IMO.

[jonpryor]

It's for saving three sizeof(void*), as jstring_wrapper has three pointer members: env, jstr, cstr.

And I only suggested using static on invalid_wrapper, not anything else.

Since invalid_wrapper will have a null env pointer, the instance is ~useless. (Also thread-safe, as all possibly mutating operations do nothing when env==nullptr.)

It's potentially "not worth it," but jstring_array_wrapper is huge: 3*sizeof(void*) + sizeof(size_t) + 6*3*sizeof(void*) == 88 bytes on a 32-bit ABI, double that on a 64-bit ABI.

On the stack.

Perhaps 88 bytes isn't as much as I fear, but IIRC we only have like 2-4KB stacks on Android...

[grendello]

The default stack size in bionic is 1MB, so I wouldn't worry too much...

[jonpryor]

Why merge the two lines into one 156-character wide line?

[jonpryor]

Welp, i suppose aborting the process on OOM isn't throwing an exception, so 👍?

[grendello]

It's a required standard overload, we must implement it. Also, we compile without exceptions anyway and also we call the new implementation above this one in the source which aborts the process on OOM so there's no issue here.

[jonpryor]

I suppose I should have been more explicit.

Given that we don't have exceptions, having operator new(size_t) abort on OOM makes sense.

However, the idea with operator new (size_t, nothrow_t) is that it can instead return nullptr on OOM instead of raising an exception. Explicit null checking is thus expected (and required!) when nothrow is used:

int* a = new (std::nothrow) int;
if (a == nullptr) {
  // OOM
  // ...
}

So inverting these could plausibly make sense: have operator new (size_t, nothrow_t) call ::malloc(), returning nullptr when malloc() returns NULL, and have operator new (size_t) call operator new (size_t, nothrow_t) and abort if nullptr is returned.

That said, it's kinda moot on Linux, as malloc() won't ever return NULL (yay OOM killer?).

I'm not sure if Android can return NULL from malloc().

[grendello]

Updated

[jonpryor]

It's truly a pity that the delete operator doesn't take a void*& parameter, so it could null out the parameter automatically...

[jonpryor]

Looks like the PR build hung:

18:59:35   Results (nunit2) saved as TestResult-Xamarin.Android.Build.Tests.xml

Note: at the time of this comment, it is 21:51, so we're approaching the 3h timeout value.

I'm aborting the build and will restart it.

(I wanted to restart the build anyway, as I added the full-mono-integration-build label...)

[jonpryor]

The most recent PR build failed in the MXE portion of src/monodroid:

  FAILED: : && /Users/builder/android-toolchain/mxe-b9cbb53/bin/x86_64-w64-mingw32.static-g++   ...
  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIEEEvjT_DpT0_: symbol wrong type (4 vs 3)
  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIiEEEvjT_DpT0_: symbol wrong type (4 vs 3)
  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIiiEEEvjT_DpT0_: symbol wrong type (4 vs 3)
  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIiiiEEEvjT_DpT0_: symbol wrong type (4 vs 3)
  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIiiiiEEEvjT_DpT0_: symbol wrong type (4 vs 3)
  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIiiiiiEEEvjT_DpT0_: symbol wrong type (4 vs 3)
  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIiiiiiiEEEvjT_DpT0_: symbol wrong type (4 vs 3)
  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIiiiiiiiEEEvjT_DpT0_: symbol wrong type (4 vs 3)
collect2 : error : ld returned 1 exit status [/Users/builder/jenkins/workspace/xamarin-android-pr-builder-release/xamarin-android/src/monodroid/monodroid.csproj]

wat?

It looks like gcc/ld doesn't like the template meta-programming within package_hash_to_hex().

There's also quite a few warnings about signed vs. unsigned integer comparisons:

src/monodroid/jni/android-system.cc(592,20): warning G971A9B88: comparison between signed and unsigned integer expressions [-Wsign-compare]
    for (int i = 0; i < AndroidSystem::MAX_OVERRIDES; i++) {
                      ^

[grendello]

Well... it seems mingw we have on bots is too old...:

  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIEEEvjT_DpT0_: symbol wrong type (4 vs 3)
  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIiEEEvjT_DpT0_: symbol wrong type (4 vs 3)
  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIiiEEEvjT_DpT0_: symbol wrong type (4 vs 3)
  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIiiiEEEvjT_DpT0_: symbol wrong type (4 vs 3)
  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIiiiiEEEvjT_DpT0_: symbol wrong type (4 vs 3)
  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIiiiiiEEEvjT_DpT0_: symbol wrong type (4 vs 3)
  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIiiiiiiEEEvjT_DpT0_: symbol wrong type (4 vs 3)
  Cannot export _ZN7xamarin7android4Util19package_hash_to_hexIiIiiiiiiiEEEvjT_DpT0_: symbol wrong type (4 vs 3)
collect2 : error : ld returned 1 exit status [/Users/builder/jenkins/workspace/xamarin-android-pr-builder-debug/xamarin-android/src/monodroid/monodroid.csproj]

[grendello]

@jonpryor gcc works just fine with the metaprogramming bit, it's mxe gcc that's too old.

[jonpryor]

Proposed commit message:

Fixes: https://github.com/xamarin/xamarin-android/issues/1443
Context: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/plot/Tests%20times/

A frequent complaint with Xamarin.Android is that application startup
times can be slower than desired.  For example, see the
[Xamarin.Forms app startup data][xf-startup], in particular the
**init-Release** column (times in milliseconds), which is the time
at which the `Runtime.init: end native-to-managed transition` message
is printed when `timing` messages are enabled.

As of Jenkins build 1359, the **init-Release** value is 518ms.

(Note that the times in that table are not necessarily useful, nor
are they future proof: they come from the emulator that the Jenkins
build machine uses, and that emulator *has* changed and *will* change
in the future.  Additionally, because it's an x86-accelerated
emulator, times are not meaningful to Android hardware devices.  The
values also fluctuate a lot.  Better than nothing? ¯\_(ツ)_/¯)

Optimize some startup operations to reduce the **init-Release** value:

  * JNI Handling Improvements:
      * Reduce Java class lookup.
      * Don't lookup `android.os.Build.VERSION.SDK_INT` via JNI.
      * `jstring` handling improvements.
  * Review logging messages.
  * Improve package name hash generation.
  * Improve environment variable processing.
  * Mono Handling Improvements
      * Stop preloading all assemblies.
      * Avoid using "system properties" to control Mono features.
  * Desktop version is now a compile-time build option, not runtime.
  * Initialize `xamarin_getifaddrs()` on-demand, not at startup.

[xf-startup]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/plot/Tests%20times/#plot-8694435842335223270.csv

~~ JNI Handling Improvements ~~

Previously, JNI was used to lookup the
`android.os.Build.VERSION.SDK_INT` value.  We now pass
`Build.VERSION.SDK_INT` into `Runtime.init()`, avoiding the JNI calls.

Previously, `JNIEnv::FindClass()` was used during process startup in
order to lookup multiple types which are required for operation.
These types are now fields on the `mono.android.Runtime` type, and
`JNIEnv::GetStaticObjectField()` is used to obtain the `jclass`.

Additionally, a handful of class field/method lookups were moved out
of the init code so that the code that doesn't use them doesn't have
to pay the tax.

`jstring` and `jobjectArray`-of-`jstring` values are now handled by
the new `jstring_wrapper` and `jstring_array_wrapper` types, which
take care of efficiently caching the retrieved strings as well as of
correctly deleting local references to the obtained objects.
Both classes are optimized so that they compile into the equivalent
of the current, hand-written, code.  They also take care to make the
minimum necessary number of calls in order to access the strings,
both standalone and from arrays, as well as to release the resources.
The string and array wrapper instances are passed around as
references, thus using the minimum amount of memory.


~~ Log Messages ~~

Previously whenever any of the `log_{info,debug}()` functions were
called we'd spend time preparing all the parameters to pass to the
function, sometimes involving memory allocation, function calls, etc.,
only to discard all of that work **inside** the `log_*` call because
the logging category used in that call was disabled.

Now we check whether the category is enabled before we set out to
construct the parameters.


~~ Environment Processing ~~

Since [2012-Aug-16][env-support], `@(AndroidEnvironment)` has worked
by creating a file named `environment` within the `.apk` -- which is
stored uncompressed within the `.apk` -- and the file is then
processed, calling **setenv**(3) to store the recorded values.

There's a fair bit of potentially hairy string manipulation here,
from ***C***, which is not entirely ideal or performant.

To speed the process up, this commit replaces the `environment` file
with a Java class generated during application build which contains
an array of `"name", "value"` pairs.  The class is passed to
`Java_mono_android_Runtime_init()` and its elements are used to
create the requested environment variables.

Some of these variables are special-cased; instead of using them for
**setenv**(3), they control flags in the `AndroidSystem` class

  * `mono.aot`: The `$(AndroidAotMode)` value; which *kind* of AOT
    mono should support at runtime.
  * `mono.llvm`: The `$(EnableLLVM)` value; whether LLVM-generated
    AOT files should be used.
  * `__XA_DSO_IN_APK`: Support in-place reading of `.so` files;
    See commit 95ca1025.

[env-support]: https://github.com/xamarin/monodroid/commit/dbd73ec8343477c8fd10379c6577a38c072a78fa


~~ Mono Handling Improvements ~~

During process startup startup, *every* assembly within the `.apk`
would be loaded so that a `Java.Interop.__TypeRegistrations` type
could be probed, and if it existed, the
`__TypeRegistrations.RegisterPackages()` method would be invoked.

This was done in order to [better support "type mappings"][typemaps]
between Java names and C# names (and vice versa).

However, this support hasn't been required since the introduction
of the [`typemap.jm` and `typemap.mj` files][typemap-files]; we just
hadn't gotten around to removing the
`__TypeRegistrations.RegisterPackages()` invocation.

*Not* loading every assembly on startup allows assemblies to be
loaded on-demand, and improves startup times.

[typemaps]: https://github.com/xamarin/xamarin-android/issues/1443#issuecomment-374343825
[typemap-files]: https://github.com/xamarin/monodroid/commit/e69b76e2b86f92fa8440756c44ad3e2205563ac7


~~ Startup Time Summary ~~

Startup times for `tests/Xamarin.Forms-Performance-Integration`,
average of 50 iterations of (uninstall app, install app, launch app):

  * Debug configuration:

        Old: 1s 440ms
        New: 1s 100ms

  * Release configuration:

        Old: 650ms
        New: 270ms

[toomasz]

Good work! When do you expect to release it?