-
Notifications
You must be signed in to change notification settings - Fork 564
Hash names of all the packaged DSOs #6522
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
Conversation
d6d10c4 to
8df92d0
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
83e4ff1 to
400ddce
Compare
| }, | ||
| }; | ||
|
|
||
| constexpr char fake_dso_name[] = "libaot-Some.Assembly.dll.so"; |
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.
Why have any DSOCacheEntry values in the "stub" build? Why not just a zero-length array? (Or does something require non-zero length arrays?)
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'm using disassembly of the stub to make sure our managed generator does the right job, for that I need it to have "valid" content.
| Output.WriteLine (); | ||
| } | ||
|
|
||
| WriteDirective (".ident", QuoteString ($"Xamarin.Android {XABuildConfig.XamarinAndroidVersion}")); |
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.
Why do we need to start emitting ident "Xamarin.Android 12.2.99"?
What do we start doing when .NET 7+ is our primary distribution vehicle and not Classic Xamarin.Android? Presumably we'll stop bumping or otherwise remove $(ProductVersion) eventually, and then use $(AndroidPackVersion) instead. (Presumably?). Should use use $(ProductVersion) for both .NET 6+ & Classic now, $(AndroidPackVersion) for both now, or selectively use one depending on which is being used?
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 don't really care which version we use, I wanted to have Xamarin.Android ${VERSION} in there, it might be helpful in diagnosing issues to know which XA built the binary. We can put git commit hash there, whatever you think makes the most sense.
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 kinda like the xamarin/xamarin-android@COMMIT-HASH idea, actually… Especially since it's "product name agnostic", and thus avoids the entire question in the first place.
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 think it still should have some "product" name, not just the hash. Xamarin.Android [HASH]?
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 "product name" of xamarin/xamarin-android@ avoids the need (responsibility) to pick a name, and since I'm being incredibly wishy-washy here, abdicating that responsibility sounds awesome.
…or we just stick with Xamarin.Android. :-).
The hash remains useful, and should be extremely helpful for future issue investigations.
...Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs
Show resolved
Hide resolved
...Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs
Show resolved
Hide resolved
...Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/EnvironmentHelper.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Build.Tasks/Utilities/ApplicationConfigNativeAssemblyGenerator.cs
Show resolved
Hide resolved
src/Xamarin.Android.Build.Tasks/Utilities/ApplicationConfigNativeAssemblyGenerator.cs
Show resolved
Hide resolved
src/Xamarin.Android.Build.Tasks/Utilities/ApplicationConfigNativeAssemblyGenerator.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Build.Tasks/Utilities/ApplicationConfigNativeAssemblyGenerator.cs
Outdated
Show resolved
Hide resolved
| string bufferLabel = GetBufferLabel (); | ||
| WriteBufferAllocation (output, bufferLabel, (uint)BundledAssemblyNameWidth); | ||
| name_labels.Add (bufferLabel); | ||
| name_labels.Add (generator.WriteEmptyBuffer ((uint)BundledAssemblyNameWidth, "env.buf")); |
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.
Another one for the naming $DEITYs, "write empty buffer"? I'm not at all sure what the semantics are based on just looking at this line.
I wonder if BeginBuffer(…) would be better? Unless this is asserting that the buffer is in fact empty, but why would we ever have known empty buffers? (I have not yet read what WriteEmptyBuffer() does. I'm not sure I'll understand what it does when I get there.)
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'm really not sure what you're asking for here. If one doesn't know what the buffer does in this context, then I don't want them to touch the code. This is the lowest level generator for a specific set of specific structures that describe the entirety of application config, and modifying the code requires understanding what the structures are on the native side. Once that is understood, the terms used will make sense. The buffer is in fact empty, because it just allocates space in the output DSO to be filled at the run time. In this case, the code is used when assembly stores are off and we read the bundled assemblies from the apk, filling in this structure:
struct XamarinAndroidBundledAssembly final
{
int32_t apk_fd;
uint32_t data_offset;
uint32_t data_size;
uint8_t *data;
uint32_t name_length;
char *name;
};the buffer is for the assembly name (as suggested by BundledAssemblyNameWidth in the method invocation) and has to be there because name has to point to something, we don't allocate data directly. All that information is there, in source code, and I do expect anyone modifying this code to familiarize themselves with the native side...
I'm really not sure how do you want to explain all that in a method name?
...amarin.Android.Build.Tasks/Utilities/NativeAssemblyGenerator/Arm32NativeAssemblyGenerator.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Build.Tasks/Utilities/NativeAssemblyGenerator/ArmNativeAssemblyGenerator.cs
Outdated
Show resolved
Hide resolved
| return String.Empty; | ||
| } | ||
|
|
||
| public virtual uint GetMaxInlineWidth (object data, string fieldName) |
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.
What is an "inline" width?
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.
Get maximum width of data buffer allocated inline (that is not pointed to)
src/Xamarin.Android.Build.Tasks/Utilities/NativeAssemblyGenerator/NativeAssemblyGenerator.cs
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Build.Tasks/Utilities/TypeMappingDebugNativeAssemblyGenerator.cs
Show resolved
Hide resolved
| base.WriteFileTop (); | ||
|
|
||
| WriteDirective (".syntax", "unified"); | ||
| WriteDirectiveWithComment (".eabi_attribute", "Tag_conformance", 67, QuoteString ("2.09")); |
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.
Where do these values come from?
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.
From assembly generated by g++ and clang, somewhat described here https://sourceware.org/binutils/docs-2.37/as/ARM-Directives.html#ARM-Directives
Not all of them apply to data-only DSOs, but I want to match output of the compilers.
src/Xamarin.Android.Build.Tasks/Utilities/NativeAssemblyGenerator/NativeAssemblyGenerator.cs
Show resolved
Hide resolved
src/Xamarin.Android.Build.Tasks/Utilities/NativeAssemblyGenerator/SymbolType.cs
Show resolved
Hide resolved
src/Xamarin.Android.Build.Tasks/Utilities/NativeAssemblyGenerator/NativeAssemblyGenerator.cs
Outdated
Show resolved
Hide resolved
| enum SectionFlags | ||
| { | ||
| None = 0, | ||
| Allocatable = 1 << 0, |
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.
Where'd these values come from?
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.
src/Xamarin.Android.Build.Tasks/Utilities/NativeAssemblyGenerator/X86NativeAssemblyGenerator.cs
Show resolved
Hide resolved
|
|
||
| if constexpr (use_precalculated_size) { | ||
| size = precalculated_size; | ||
| log_warn (LOG_ASSEMBLY, "Pre-calculated entry size = %u", size); |
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.
Should this be log_warn() and not log_info()?
761f598 to
a00c2d5
Compare
|
Proposed commit message: [monodroid] Hash the names of all packaged DSOs (#6522)
Context: https://github.com/xamarin/xamarin-android/pull/6522
Context: https://sourceware.org/binutils/docs-2.37/as/index.html
Context: https://sourceware.org/binutils/docs-2.37/as/AArch64-Directives.html#AArch64-Directives
Context: https://sourceware.org/binutils/docs-2.37/as/ARM-Directives.html#ARM-Directives
Context: https://sourceware.org/binutils/docs-2.37/as/Quad.html#Quad
Commit 000cf5a8 introduced hashing of native library names so that
we would only try to load specific Mono components which were
configured at app build time.
Expand this concept so that we support hashing any form of native
library name (DSO name) sent to us by Mono, and look for a cached
entry for that DSO. The cache includes the actual native library
name, whether it should be ignored when requested (e.g. an empty AOT
`.so` file; see db161ae7 & df667b07), and the cached `dlopen()` value:
struct DSOCacheEntry {
uint64_t hash;
bool ignore;
const char *name;
void *handle;
};
DSOCacheEntry dso_cache[] = {…};
The `dso_cache` is computed at App build time, and stored in
`libxamarin-app.so`, and contains values sorted on
`DSOCacheEntry::hash`.
The use of `dso_cache` removes a bit of string processing from both
startup and the application run time, reducing app startup times in
some scenarios:
| Scenario | Before ms | After ms | Δ |
| --------------------------------------------- | --------: | --------: | -------: |
| Plain Xamarin.Android (JIT, 64-bit) | 311.500 | 311.600 | +0.03% ✗ |
| Plain Xamarin.Android (Profiled AOT, 64-bit) | 253.500 | 247.000 | -2.56% ✓ |
| .NET MAUI Hello World (JIT, 64-bit) | 1156.300 | 1159.500 | +0.28% ✗ |
| .NET MAUI Hello World (Profiled AOT, 64-bit) | 868.900 | 831.700 | -4.28% ✓ |
(Times measured on a Pixel 3 XL.)
Above table is a subset of values from xamarin/xamarin-android#6522;
see original PR for complete table information. We believe that the
occasional increases are within the margin of error.
While implementing the above described `dso_cache` idea, I hit known
limitations of the native assembler generator code which, until this
commit, weren't a problem (mostly related to hard-coded structure and
array alignment). Address these limitations by rewriting the assembly
generator. It now fully implements the structure and symbol alignment
calculation for all the supported architectures. Also added is the
ability to generate assembler code from managed structures, using
reflection, without the need of manually written code. It also fixes
a previously unnoticed issue which made typemap structures not aligned
properly, which may have made them slightly slower than necessary. |
|
Regarding the TODOs above, the differences in both mentioned cases are so small that I chalked it up to measurement error (especially for Displayed times which is inherently unreliable), but JIT may be affected by external factors on device as well. The tests are executed on a "live" device which does a lot in the background, any form of I/O may affect the tests performance. Over the time I've been working on performance, I learned that differences of around 1% can largely be ignored and accepted as "no change" |
Use the hash to map any form of DSO name sent to us by Mono (we generate hashes for several mutations) to find a cache entry for that DSO. Cache includes the actual DSO name, whether it should be ignored when requested (true for empty AOT DSOs) and the cached module handle. This removes a bit of string processing from both startup and the application run time. Timing TBD
There's no need to implement support for arrays of all types, or for IntPtr - it can be easily added if it's needed, no point in having dead code
This PR adds a comment section to
libxamarin-app.sothe branchand commit hash of Xamarin.Android which produced the shared library.
The information can be displayed using
llvm-objdumporobjdumpas follows (parameters for both commands are the same):
Use the hash to map any form of DSO name sent to us by Mono (we generate
hashes for several mutations) to find a cache entry for that DSO. Cache
includes the actual DSO name, whether it should be ignored when
requested (true for empty AOT DSOs) and the cached module handle.
This removes a bit of string processing from both startup and the
application run time.
This commit started with the goal to implement the above, but during
implementation of the DSO cache I hit known limitations of the native
assembler generator code which, until this commit weren't a problem
(mostly related to hard-coded structure and array alignment). So now
it includes a new implementation of the said generator, this time it
fully implements the structure and symbol alignment calculation for
all the supported architectures. Also added is the ability to generate
assembler code from managed structures, using reflection, without the need
of manually written code. It also fixes previously unnoticed issue which
made typemap structures not aligned properly, which may have made them
slightly slower than necessary
The implemented changes improve startup of both plain XA and MAUI applications,
measured on Pixel 3 XL running Android 12:
Plain XA application
AOT, Displayed time
AOT, Total Initialization Time
No AOT, Displayed time
No AOT, Total Init Time
MAUI Hello World
AOT, Displayed time
AOT, Total Init Time
No AOT, Displayed time
No AOT, Total Init Time