Tune bundled assemblies #6188
Merged
Tune bundled assemblies #6188
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
tune-bundled-assemblies
branch
3 times, most recently
from
8932a07 to
30cd53e
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
dellis1972
approved these changes
Aug 17, 2021
grendello
force-pushed
the
tune-bundled-assemblies
branch
from
30cd53e to
ec58ab2
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jonpryor
reviewed
Aug 17, 2021
| @@ -295,6 +321,7 @@ void AddEnvironment () | |||
| JniAddNativeMethodRegistrationAttributePresent = appConfState != null ? appConfState.JniAddNativeMethodRegistrationAttributePresent : false, | |||
| HaveRuntimeConfigBlob = haveRuntimeConfigBlob, | |||
| NumberOfAssembliesInApk = assemblyCount, | |||
| BundledAssemblyNameWidth = assemblyNameWidth + 1, | |||
There was a problem hiding this comment.
Why the +1 here? Terminating NUL? If so, should ApplicationConfigNativeAssemblyGenerator do that addition instead?
There was a problem hiding this comment.
No, ApplicationConfigNativeAssemblyGenerator doesn't calculate the value - its task is just to write it.
jonpryor
reviewed
Aug 17, 2021
|
|
||
| int64_t assembly_count = application_config.number_of_assemblies_in_apk; | ||
| bool bundled_assemblies_slow_path = bundled_assembly_index >= application_config.number_of_assemblies_in_apk; | ||
| uint32_t max_assembly_name_size = application_config.bundled_assembly_name_width - 1; |
There was a problem hiding this comment.
This is "differently" amusing: we have to do +1 to support the terminating NUL, yet when we hit native code we just -1 that value.
Do we actually need the terminating NUL?
Probably, actually?
The set_entry_data<false>() method uses max_assembly_name_size as the len parameter to strncpy(3), and strncpy() won't read or write past len, it feels like there could be a situation where XamarinAndroidBundledAssembly::name isn't properly NUL-terminated?
There was a problem hiding this comment.
The NUL is just a failsafe, just as is max_assembly_name_size. See this comment for more explanation.
jonpryor
reviewed
Aug 17, 2021
| if constexpr (NeedsNameAlloc) { | ||
| entry.name = utils.strdup_new (entry_name.get () + prefix_len); | ||
| } else { | ||
| strncpy (entry.name, entry_name.get () + prefix_len, max_name_size); |
There was a problem hiding this comment.
Should this also explicitly set entry.name[max_name_size]=0 or entry.name[max_name_size-1]=0?
Given the +1/-1 operations elsewhere, I think libxamarin-app.so will allocate the XamarinAndroidBundledAssembly::name entry to be of length char[max_name_size+1] (due to the -1 in libmonodroid.so, and thus it'll come "pre-NUL-terminated", but it's taken me more mental effort than I'd like to admit to come to this conclusion.
There was a problem hiding this comment.
strncpy copies at most n bytes (in this case max_name_size) which may not include the terminating NUL byte, true, but since this line is used only with pre-allocated buffers, the string is terminated properly even if the name takes the entire max_name_size characters (since the name buffer is always max_name_size + 1 long). strncpy also stops copying at the first \0 it encounters and copies it to the destination buffer, if it's found within the first n bytes of the source string.
jonpryor
reviewed
Aug 17, 2021
| Action<ITaskItem> updateNameWidth = (ITaskItem assembly) => { | ||
| string assemblyName = Path.GetFileName (assembly.ItemSpec); | ||
| if (assemblyName.Length > assemblyNameWidth) { | ||
| assemblyNameWidth = assemblyName.Length; |
There was a problem hiding this comment.
I like the idea, but this isn't "Unicode"-aware: for better or worse, this is entirely valid:
% csc /out:😀.exe hw.cs
% mono 😀.exe
Hello, world!
or for the .csproj-inclined:
<PropertyGroup>
<AssemblyName>😀</AssemblyName>
</PropertyGroup>On macOS, what is 😀?
% ls -l | tail -1 | hexdump -C
00000000 2d 72 77 2d 72 2d 2d 72 2d 2d 20 20 20 20 31 20 |-rw-r--r-- 1 |
00000010 6a 6f 6e 20 20 73 74 61 66 66 20 20 20 20 20 20 |jon staff |
00000020 33 35 38 34 20 41 75 67 20 31 37 20 31 39 3a 33 |3584 Aug 17 19:3|
00000030 32 20 f0 9f 98 80 2e 65 78 65 0a |2 .....exe.|
i.e. 😀 is { f0 9f 98 80 }, which is 4 bytes, not 1.
You shouldn't assume that assembly names are restricted to ASCII.
Is this even possible in Xamarin.Android? I don't know about emoji, but we do use umlauts within directory names: https://github.com/xamarin/xamarin-android/blob/f7d32102cc3ada67aa12cc0beed2cf97226f41b3/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/AotTests.cs#L159
…which means we should improve that test (and others?) so that assemblies also contain umlauts. Pity we missed that opportunity.
There was a problem hiding this comment.
There was a problem hiding this comment.
Good point, I keep forgetting that people can do weird things (especially with emoji) :)
jonpryor
reviewed
Aug 17, 2021
| @@ -90,6 +92,9 @@ protected override void WriteSymbols (StreamWriter output) | |||
| WriteCommentLine (output, "number_of_assemblies_in_apk"); | |||
| size += WriteData (output, NumberOfAssembliesInApk); | |||
|
|
|||
| WriteCommentLine (output, "bundled_assembly_name_width"); | |||
| size += WriteData (output, BundledAssemblyNameWidth); | |||
There was a problem hiding this comment.
e.g. perhaps this should contain +1 /* NUL */?
There was a problem hiding this comment.
No, as said above, this class merely generates the native assembly - value calculation is not its concern.
jonpryor
reviewed
Aug 17, 2021
| var name_labels = new List<string> (); | ||
| for (int i = 0; i < NumberOfAssembliesInApk; i++) { | ||
| string bufferLabel = GetBufferLabel (); | ||
| WriteBufferAllocation (output, bufferLabel, (uint)BundledAssemblyNameWidth); |
There was a problem hiding this comment.
Ditto here (uint) BundledAssemblyNameWidth+1 /* NUL */?
grendello
force-pushed
the
tune-bundled-assemblies
branch
from
ec58ab2 to
25a2032
Compare
|
The test failures don't appear to be caused by this PR. I managed to reproduce them locally using the tip of I have no idea why the designer tests crash though... They shouldn't be affected by this PR, which is about code that runs on Android device. |
|
WebSocket tests will be disabled by this PR |
C++ features: * Use C++20 concepts to make compile-time correctness verification stronger * Use `std::vector` for dynamic heap buffers * Use `std::array` for static stack buffers * Use `std::unique_ptr` instead of the home-grown equivalent (also for C-allocated memory) * Use C++20 structure designators Optimizations: * Add string routines (`ends_with` and `find_last`) optimized for the stack string types (derived from `string_base`) * Count assemblies at the build time and pre-allocate enough memory (in the form of `std::vector` instance) to store information about the bundled assemblies, thus avoiding 2 allocations per assembly (one `realloc` and one `malloc`) making the bundled assemblies loading algorithm closer to O(1) instead of the previous O(n*2) * Use character arrays instead of "plain" string literals to optimize some string operations. These changes improve plain XA test app startup time by around 11%
Bundled assemblies are now stored on run time in an array pre-allocated
during build. This allows us to avoid any memory allocation during
startup. At the same time, a provision is made for adding more
assemblies than those counted and placed in the .apk during the build.
This may come handy when Xamarin.Android supports split
applications (with components placed in different APKs) and also as a
simple fail-safe feature should the build miscalculate something.
Debug information for assemblies is no longer registered when reading
the APK, but rather whenever an assembly load is requested by Mono.
Neither the assemblies nor their debug information are mmapped when
reading the APK anymore. This is done lazily when assembly load is
requested by Mono. Mapping of files after the startup phase is protected
with a POSIX semaphore.
Additionally, a handful of simple optimizations are implemented:
* we no longer query the device's memory page size whenever we mmap a
file, the value is instead queried and cached in the `Util`
constructor.
* current app domain is no longer queried when getting a Mono object's
type under NET6
* a number of log statements during startup no longer run by
default
The changes reduce startup time for a simple (one control in a layout)
plain Xamarin.Android application by around 4ms measured for `Displayed`
time, and around 2ms when measured for native runtime startup.
For the `Hello Maui` sample from the `maui-samples` repository, the
startup time is reduced by ~200ms measured for `Displayed` time (from
~1.6s to ~1.4s), with the same speed up for the native runtime as in the
plain Xamarin.Android test above.
Performance was measured on a Pixel 3XL phone running Android 12 beta -
the `Displayed` measurements were found to be **very** unstable during
testing (with variance between runs reaching 600ms sometimes).
grendello
force-pushed
the
tune-bundled-assemblies
branch
from
25a2032 to
466f40c
Compare
When Embedded Assemblies are used (`$(EmbedAssembliesIntoApk)`=True),
assembly names are detected during `.apk` content traversal,
duplicated, and stored into `bundled_assemblies` and
`MonoBundledAssembly::name`.
Reduce memory allocations by updating `libxamarin-app.so` to contain
a `bundled_assembly_names` array, a'la:
struct XamarinAndroidBundledAssembly {
int32_t apk_fd;
uint32_t data_offset;
uint32_t data_size;
uint8_t *data;
uint32_t name_length;
char *name;
};
constexpr size_t AssemblyNameWidth;
static char assembly_name_1 [AssemblyNameWidth];
static char assembly_name_2 [AssemblyNameWidth];
// …
extern "C" XamarinAndroidBundledAssembly bundled_assemblies[] = {
{
.apk_fd = -1,
.data_offset = 0,
.data_size = 0,
.name_length = 0,
.name = assembly_name_1,
},
{
.apk_fd = -1,
.data_offset = 0,
.data_size = 0,
.name_length = 0,
.name = assembly_name_2,
},
// …
}
The `bundled_assemblies` array *doesn't* contain the assembly names
detected during packaging time. Rather, it contains *buffers*, each
large enough to hold any assembly name.
This removes the need to allocate additional memory for assembly
names during process startup. (We'll still need to allocate memory
for other entries within the `.apk`, such as `.pdb` files.)
Additionally, a provision is made for adding more assemblies than
those counted and placed in the .`apk` during the build. This may
come handy when Xamarin.Android supports split applications (with
components placed in different `.apk` files), and also as a simple
fail-safe feature should the build miscalculate something.
Debug information for assemblies is no longer registered when reading
the APK, but rather whenever an assembly load is requested by Mono.
Neither the assemblies nor their debug information are mmapped when
reading the APK anymore. This is done lazily when assembly load is
requested by Mono. Mapping of files after the startup phase is
protected with a POSIX semaphore.
Additionally, a handful of simple optimizations are implemented:
* We no longer query the device's memory page size whenever we
`mmap()` a file; the value is instead queried and cached in the
`Util` constructor.
* The current AppDomain is no longer queried when getting a Mono
object's type under .NET 6
* Many log statements are no longer run by default during startup.
These changes reduce startup time for a simple plain
Xamarin.Android application (one control in a layout) by around 4ms
measured for `Displayed` time, and by around 2ms when measured for
native runtime startup.
For the `Hello Maui` sample from the `maui-samples` repository, the
startup time is reduced by ~200ms measured for `Displayed` time (from
~1.6s to ~1.4s), with the same speed up for the native runtime as in
the plain Xamarin.Android test above.
Performance was measured on a Pixel 3XL phone running Android 12 beta.
*Note*: the `Displayed` measurements were found to be **very** unstable
during testing on Android 12, with variance between runs reaching 600ms
sometimes. |
| void prepare_for_multiple_threads () noexcept | ||
| { | ||
| int ret = sem_init (&assembly_mmap_semaphore, 0 /* pshared */, 1 /* value */); | ||
| abort_unless (ret == 0, "Failed to initialize assembly mapping semaphore. %s", strerror (errno)); |
There was a problem hiding this comment.
I think this abort is happening during the Designer tests since this got merged?
I got this from their log, is it useful?
[2021-08-18 23:45:32.5] Renderer (error) >> ../../../jni/embedded-assemblies.hh:89 (prepare_for_multiple_threads): Failed to initialize assembly mapping semaphore. Function not implemented
[2021-08-18 23:45:32.5] Renderer >>
[2021-08-18 23:45:32.5] Renderer >> =================================================================
[2021-08-18 23:45:32.5] Renderer >> Native Crash Reporting
[2021-08-18 23:45:32.5] Renderer >> =================================================================
[2021-08-18 23:45:32.5] Renderer >> Got a abrt while executing native code. This usually indicates
[2021-08-18 23:45:32.5] Renderer >> a fatal error in the mono runtime or one of the native libraries
[2021-08-18 23:45:32.5] Renderer >> used by your application.
[2021-08-18 23:45:32.5] Renderer >> =================================================================
[2021-08-18 23:45:32.5] Renderer >>
[2021-08-18 23:45:32.5] Renderer >> =================================================================
[2021-08-18 23:45:32.5] Renderer >> Native stacktrace:
[2021-08-18 23:45:32.5] Renderer >> =================================================================
[2021-08-18 23:45:32.5] Renderer >> 0x110bfc9d8 - /Library/Frameworks/Xamarin.Android.framework/Versions/11.4.99.108/lib/xamarin.android/xbuild/Xamarin/Android/lib/host-Darwin/libmonosgen-2.0.dylib : mono_dump_native_crash_info
[2021-08-18 23:45:32.5] Renderer >> 0x110b9738e - /Library/Frameworks/Xamarin.Android.framework/Versions/11.4.99.108/lib/xamarin.android/xbuild/Xamarin/Android/lib/host-Darwin/libmonosgen-2.0.dylib : mono_handle_native_crash
[2021-08-18 23:45:32.5] Renderer >> 0x110bfbfdf - /Library/Frameworks/Xamarin.Android.framework/Versions/11.4.99.108/lib/xamarin.android/xbuild/Xamarin/Android/lib/host-Darwin/libmonosgen-2.0.dylib : sigabrt_signal_handler
[2021-08-18 23:45:32.5] Renderer >> 0x7fff20438d7d - /usr/lib/system/libsystem_platform.dylib : _sigtramp
[2021-08-18 23:45:32.5] Renderer >> 0x1123c1468 - Unknown
[2021-08-18 23:45:32.5] Renderer >> 0x7fff20348406 - /usr/lib/system/libsystem_c.dylib : abort
[2021-08-18 23:45:32.5] Renderer >> 0x110ab2cbd - /Library/Frameworks/Xamarin.Android.framework/Versions/11.4.99.108/lib/xamarin.android/xbuild/Xamarin/Android/lib/host-Darwin/libmono-android.debug.dylib : _ZL15do_abort_unlessbPKcz
[2021-08-18 23:45:32.5] Renderer >> 0x110aafa7a - /Library/Frameworks/Xamarin.Android.framework/Versions/11.4.99.108/lib/xamarin.android/xbuild/Xamarin/Android/lib/host-Darwin/libmono-android.debug.dylib : _ZN7xamarin7android8internal16MonodroidRuntime38Java_mono_android_Runtime_initInternalEP7JNIEnv_P7_jclassP8_jstringP13_jobjectArrayS8_SA_P8_jobjectSA_ih
[2021-08-18 23:45:32.5] Renderer >> 0x110aaf005 - /Library/Frameworks/Xamarin.Android.framework/Versions/11.4.99.108/lib/xamarin.android/xbuild/Xamarin/Android/lib/host-Darwin/libmono-android.debug.dylib : Java_mono_android_Runtime_init
[2021-08-18 23:45:32.5] Renderer >> 0x1124ca3a7 - Unknown
[2021-08-18 23:45:32.5] Renderer >> 0x1124b9ffd - Unknown
[2021-08-18 23:45:32.5] Renderer >> 0x1124b97d0 - Unknown
[2021-08-18 23:45:32.5] Renderer >>
[2021-08-18 23:45:32.5] Renderer >> =================================================================
[2021-08-18 23:45:32.5] Renderer >> Basic Fault Address Reporting
[2021-08-18 23:45:32.5] Renderer >> =================================================================
[2021-08-18 23:45:32.5] Renderer >> Memory around native instruction pointer (0x7fff203c492e):0x7fff203c491e ff ff c3 90 90 90 b8 48 01 00 02 49 89 ca 0f 05 .......H...I....
[2021-08-18 23:45:32.5] Renderer >> 0x7fff203c492e 73 08 48 89 c7 e9 a1 a1 ff ff c3 90 90 90 b8 53 s.H............S
[2021-08-18 23:45:32.5] Renderer >> 0x7fff203c493e 00 00 02 49 89 ca 0f 05 73 08 48 89 c7 e9 89 a1 ...I....s.H.....
[2021-08-18 23:45:32.5] Renderer >> 0x7fff203c494e ff ff c3 90 90 90 b8 83 01 00 02 49 89 ca 0f 05 ...........I....
[2021-08-18 23:45:32.5] Renderer >>
[2021-08-18 23:45:32.5] Renderer >> =================================================================
There was a problem hiding this comment.
It does help, thanks!
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.
Bundled assemblies are now stored on run time in an array pre-allocated
during build. This allows us to avoid any memory allocation during
startup. At the same time, a provision is made for adding more
assemblies than those counted and placed in the .apk during the build.
This may come handy when Xamarin.Android supports split
applications (with components placed in different APKs) and also as a
simple fail-safe feature should the build miscalculate something.
Debug information for assemblies is no longer registered when reading
the APK, but rather whenever an assembly load is requested by Mono.
Neither the assemblies nor their debug information are mmapped when
reading the APK anymore. This is done lazily when assembly load is
requested by Mono. Mapping of files after the startup phase is protected
with a POSIX semaphore.
Additionally, a handful of simple optimizations are implemented:
file, the value is instead queried and cached in the
Utilconstructor.
type under NET6
default
The changes reduce startup time for a simple (one control in a layout)
plain Xamarin.Android application by around 4ms measured for
Displayedtime, and around 2ms when measured for native runtime startup.
For the
Hello Mauisample from themaui-samplesrepository, thestartup time is reduced by ~200ms measured for
Displayedtime (from~1.6s to ~1.4s), with the same speed up for the native runtime as in the
plain Xamarin.Android test above.
Performance was measured on a Pixel 3XL phone running Android 12 beta -
the
Displayedmeasurements were found to be very unstable duringtesting (with variance between runs reaching 600ms sometimes).