-
Notifications
You must be signed in to change notification settings - Fork 538
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
[NativeAOT] Generate optimized type mapping #9856
base: main
Are you sure you want to change the base?
[NativeAOT] Generate optimized type mapping #9856
Conversation
_hashingAssemblyLoadContext = new AssemblyLoadContext (name: null, isCollectible: true); | ||
_hashMethod = GetHashMethod (_hashingAssemblyLoadContext, assemblyPath); |
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.
So, this is because we had trouble getting illink to load an assembly reference of a trimmer step?
Was the problem, just getting it to load? Or is a different System.IO.Hashing.dll
already loaded in the process and being used during illink?
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.
Was the problem, just getting it to load?
Yes, the problem was that it wouldn't load it at all. I tried copying the assembly into several locations, where I expected the assembly loader to look for the dependencies, but I couldn't get it to load.
ILLink uses AssemblyLoadContext.LoadFromAssemblyPath
internally and it appears that it won't load other referenced assemblies if they aren't included in the trimmer's deps.json
file?
@sbomer do you have any suggestions how to make sure the System.IO.Hashing.dll
dependency of our custom liker step is loaded without doing it automatically?
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 wondered if it would load if you add the assembly to @(_TrimmerCustomSteps)
with no metadata:
Line 47 in 60eb9de
<_TrimmerCustomSteps Include="$(_AndroidLinkerCustomStepAssembly)" Type="Microsoft.Android.Sdk.ILLink.PreserveSubStepDispatcher" /> |
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.
As far as I know LoadFromAssemblyPath
doesn't resolve dependencies, and we don't want to be in the business of handling potential conflicts between custom step dependencies, so a custom ALC is probably the best supported way to do this. Adding a custom step with the dependency might get it to load, but I think will cause trimming to fail when the custom step isn't found.
src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/TypeMapping.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/TypeMapping.cs
Outdated
Show resolved
Hide resolved
410116e
to
99a21e7
Compare
|
||
// Create static field to store the raw bytes | ||
var bytesField = new FieldDefinition ("s_hashes", FieldAttributes.Private | FieldAttributes.Static | FieldAttributes.InitOnly, arrayType); | ||
bytesField.InitialValue = hashes.Select(h => BitConverter.GetBytes (h)).SelectMany (x => x).ToArray (); |
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.
Are there endianness issues here? For example, will the host (IL generator) and target always have the same endianness?
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.
You're right. I need to revisit this.
throw new InvalidOperationException ($"The '{SystemIOHashingAssemblyPathCustomData}' custom data must point to a valid assembly path ('{assemblyPath}' does not exist)"); | ||
} | ||
|
||
_hashingAssemblyLoadContext = new AssemblyLoadContext (name: null, isCollectible: true); |
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.
If you do not come up with something better, you can just use Assembly.LoadFile
here. Assembly.LoadFile
does exactly what you are doing here: load an assembly into its own isolated AssemblyLoadContext.
Co-authored-by: Aaron Robinson <arobins@microsoft.com> Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@jonathanpeppers, @pjcollins: any idea why
|
We automatically try to pull in .pdb files for all
|
src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/TypeMapping.cs
Outdated
Show resolved
Hide resolved
{ | ||
ulong hash = Hash (javaClassName); | ||
|
||
// the hashes array is sorted and all the hashes are unique |
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.
Are you going to check the uniqueness of the hashcodes at build time to guarantee this?
An alternative way to deal with this would be to allow non-unique hashcodes and check all candidates in the map. If you do that, you can change the hashcodes to 32-bit that makes the map 2x smaller and dealing with hashcodes faster, at the cost of a rare hash collision.
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.
Are you going to check the uniqueness of the hashcodes at build time to guarantee this?
Yes, the check is here: https://github.com/dotnet/android/pull/9856/files#diff-8e22a99ebc58c9417a48013888fc90d72fa6b638aff5d2d449cbf2c78bd0a8fbR149-R161
If you do that, you can change the hashcodes to 32-bit that makes the map 2x smaller and dealing with hashcodes faster, at the cost of a rare hash collision.
That's definitely worth trying. Assuming even the XxHash32 collisions are very rare, in the typical case there should be just 1 string comparison to verify that the hash belongs to the expected result.
…AOT/TypeMapping.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>
This PR replaces the current managed type mapping dictionary, which needs to be fully build at startup by individually calling
Add
methods, with a pre-generated static array of class name hashes.The array of hashes is sorted so we can use binary search to find the index corresponding to any class name. This idea is not new, it is exacatly how the native type maps work at the moment.
Once we know the index of a class, we can use a generated IL switch to look up the corresponding
Type
. I chose to generate this jumptable in code because this is what Native AOT understands best. Constructing a static array of metadata tokens is AFAIK not an option for Native AOT.The generated IL is conceptually equivalent to the following C# code:
The
s_hashes
field is stored as a byte buffer in the assembly and it is loaded from the static RVA field under<PrivateImplementationDetails>
. The relevant generated code looks like this for a simple app (samples/NativeAOT.csproj
):Notes
TypeManager.GetSimpleReferences
method CAN return multiple values. It's not clear to me when this can happen (invokers?) and if the inverse type mapping can also be just 1:1. If that were the case, the related code could be revisited and optimized. I at least added caching for the time being because this method is repeatedly called with the same inputs at the startup of MAUI apps./cc @ivanpovazan @vitek-karas @AaronRobinsonMSFT