-
Notifications
You must be signed in to change notification settings - Fork 569
[TrimmableTypeMap] Use Crc64 package naming by default with LowercaseCrc64 compatibility #11193
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
5e1adf6
9d16443
174f08f
1049678
ec92067
8d5709b
05cebc7
3722fd9
e9ae72c
fa8493d
ab689ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| using System.Reflection.Metadata; | ||
| using System.Reflection.Metadata.Ecma335; | ||
| using System.Reflection.PortableExecutable; | ||
| using Java.Interop.Tools.JavaCallableWrappers; | ||
| namespace Microsoft.Android.Sdk.TrimmableTypeMap; | ||
|
|
||
| /// <summary> | ||
|
|
@@ -16,8 +17,19 @@ namespace Microsoft.Android.Sdk.TrimmableTypeMap; | |
| /// </summary> | ||
| public sealed class JavaPeerScanner : IDisposable | ||
| { | ||
| enum HashedPackageNamingPolicy { | ||
| XxHash64, | ||
| LowercaseCrc64, | ||
| } | ||
|
|
||
| readonly Dictionary<string, AssemblyIndex> assemblyCache = new (StringComparer.Ordinal); | ||
| readonly Dictionary<(string typeName, string assemblyName), ActivationCtorInfo> activationCtorCache = new (); | ||
| readonly HashedPackageNamingPolicy packageNamingPolicy; | ||
|
|
||
| public JavaPeerScanner (string? packageNamingPolicy = null) | ||
| { | ||
| this.packageNamingPolicy = ParsePackageNamingPolicy (packageNamingPolicy); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Resolves a type name + assembly name to a TypeDefinitionHandle + AssemblyIndex. | ||
|
|
@@ -913,7 +925,7 @@ static string GetJavaAccess (MethodAttributes access) | |
| return registerJniName; | ||
| } | ||
|
|
||
| // Fall back to already-scanned results (component-attributed or CRC64-computed peers) | ||
| // Fall back to already-scanned results (component-attributed or hashed-package peers) | ||
| if (results.TryGetValue (baseTypeName, out var basePeer)) { | ||
| return basePeer.JavaName; | ||
| } | ||
|
|
@@ -1331,12 +1343,12 @@ bool ExtendsJavaPeer (TypeDefinition typeDef, AssemblyIndex index) | |
|
|
||
| /// <summary> | ||
| /// Compute both JNI name and compat JNI name for a type without [Register] or component Name. | ||
| /// JNI name uses CRC64 hash of "namespace:assemblyName" for the package. | ||
| /// JNI name uses the selected package naming policy hash for "namespace:assemblyName". | ||
| /// Compat JNI name uses the raw managed namespace (lowercased). | ||
| /// If a declaring type has [Register], its JNI name is used as prefix for both. | ||
| /// Generic backticks are replaced with _. | ||
| /// </summary> | ||
| static (string jniName, string compatJniName) ComputeAutoJniNames (TypeDefinition typeDef, AssemblyIndex index) | ||
| (string jniName, string compatJniName) ComputeAutoJniNames (TypeDefinition typeDef, AssemblyIndex index) | ||
| { | ||
| var (typeName, parentJniName, ns) = ComputeTypeNameParts (typeDef, index); | ||
|
|
||
|
|
@@ -1345,7 +1357,7 @@ bool ExtendsJavaPeer (TypeDefinition typeDef, AssemblyIndex index) | |
| return (name, name); | ||
| } | ||
|
|
||
| var packageName = GetCrc64PackageName (ns, index.AssemblyName); | ||
| var packageName = GetHashedPackageName (ns, index.AssemblyName); | ||
| var jniName = $"{packageName}/{typeName}"; | ||
|
|
||
| string compatName = ns.Length == 0 | ||
|
|
@@ -1360,7 +1372,7 @@ bool ExtendsJavaPeer (TypeDefinition typeDef, AssemblyIndex index) | |
| /// registered JNI name or the outermost namespace. | ||
| /// Matches JavaNativeTypeManager.ToJniName behavior: walks up declaring types | ||
| /// and if a parent has [Register] or a component attribute JNI name, uses that | ||
| /// as prefix instead of computing CRC64 from the namespace. | ||
| /// as prefix instead of computing hashed package names from the namespace. | ||
| /// </summary> | ||
| static (string typeName, string? parentJniName, string ns) ComputeTypeNameParts (TypeDefinition typeDef, AssemblyIndex index) | ||
| { | ||
|
|
@@ -1465,16 +1477,51 @@ static void ParseConnectorDeclaringType (string? connector, out string declaring | |
| declaringAssemblyName = nextComma >= 0 ? rest.Substring (0, nextComma).Trim () : rest.Trim (); | ||
| } | ||
|
|
||
| static string GetCrc64PackageName (string ns, string assemblyName) | ||
| string GetHashedPackageName (string ns, string assemblyName) | ||
| { | ||
| // Only Mono.Android preserves the namespace directly | ||
| if (assemblyName == "Mono.Android") { | ||
| return ns.ToLowerInvariant ().Replace ('.', '/'); | ||
| } | ||
|
|
||
| var data = System.Text.Encoding.UTF8.GetBytes ($"{ns}:{assemblyName}"); | ||
| var hash = System.IO.Hashing.Crc64.Hash (data); | ||
| return $"crc64{BitConverter.ToString (hash).Replace ("-", "").ToLowerInvariant ()}"; | ||
| return packageNamingPolicy switch { | ||
| HashedPackageNamingPolicy.LowercaseCrc64 => "crc64" + ToLegacyCrc64 (ns + ":" + assemblyName), | ||
| _ => "xx64" + ToXxHash64 (ns + ":" + assemblyName), | ||
| }; | ||
| } | ||
|
|
||
| static HashedPackageNamingPolicy ParsePackageNamingPolicy (string? packageNamingPolicy) | ||
| { | ||
| if (string.Equals (packageNamingPolicy, "LowercaseCrc64", StringComparison.OrdinalIgnoreCase)) { | ||
| return HashedPackageNamingPolicy.LowercaseCrc64; | ||
| } | ||
|
|
||
| return HashedPackageNamingPolicy.XxHash64; | ||
| } | ||
|
|
||
| static string ToLegacyCrc64 (string value) | ||
| { | ||
| var data = System.Text.Encoding.UTF8.GetBytes (value); | ||
| var hash = Crc64Helper.Compute (data); | ||
| var buf = new char [hash.Length * 2]; | ||
| int i = 0; | ||
| foreach (var b in hash) { | ||
| buf [i++] = GetHexLowerChar (b >> 4); | ||
| buf [i++] = GetHexLowerChar (b & 0xF); | ||
| } | ||
| return new string (buf); | ||
| } | ||
|
|
||
| static string ToXxHash64 (string value) | ||
| { | ||
| var data = System.Text.Encoding.UTF8.GetBytes (value); | ||
| var hash = System.IO.Hashing.XxHash64.Hash (data); | ||
| return BitConverter.ToString (hash).Replace ("-", "").ToLowerInvariant (); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot we should take inspiration from dotnet/android-tools#335 and avoid so many allocations here - use stackalloc where possible and non-allocating method overloads. Make sure to reuse the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented in 8d5709b: reduced allocations in |
||
| } | ||
|
|
||
| static char GetHexLowerChar (int value) | ||
| { | ||
| return (char) (value < 10 ? ('0' + value) : ('a' + value - 10)); | ||
| } | ||
|
|
||
| static string ExtractNamespace (string fullName) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ | |
| <AndroidVersionCodePattern Condition=" '$(AndroidUseLegacyVersionCode)' != 'True' And '$(AndroidVersionCodePattern)' == '' ">{abi}{versionCode:D5}</AndroidVersionCodePattern> | ||
| <AndroidResourceGeneratorTargetName>UpdateGeneratedFiles</AndroidResourceGeneratorTargetName> | ||
| <AndroidUseApkSigner Condition=" '$(AndroidUseApkSigner)' == '' ">True</AndroidUseApkSigner> | ||
| <AndroidPackageNamingPolicy Condition=" '$(AndroidPackageNamingPolicy)' == '' ">LowercaseCrc64</AndroidPackageNamingPolicy> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 05cebc7: reverted the global |
||
| <AndroidPackageNamingPolicy Condition=" '$(AndroidPackageNamingPolicy)' == '' ">XxHash64</AndroidPackageNamingPolicy> | ||
| <AndroidUseManagedDesignTimeResourceGenerator Condition=" '$(AndroidUseManagedDesignTimeResourceGenerator)' == '' And '$(OS)' != 'Windows_NT' ">False</AndroidUseManagedDesignTimeResourceGenerator> | ||
| <BundleToolVersion Condition="'$(BundleToolVersion)' == ''">@BUNDLETOOL_VERSION@</BundleToolVersion> | ||
| <_XamarinAndroidMSBuildDirectory>$(MSBuildThisFileDirectory)</_XamarinAndroidMSBuildDirectory> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,12 +72,12 @@ static string[]? AllUserTypesAssemblyPaths { | |
| } | ||
| } | ||
|
|
||
| static string NormalizeCrc64 (string javaName) | ||
| static string NormalizeHashedPackageName (string javaName) | ||
| { | ||
| if (javaName.StartsWith ("crc64", StringComparison.Ordinal)) { | ||
| if (javaName.StartsWith ("crc64", StringComparison.Ordinal) || javaName.StartsWith ("xx64", StringComparison.Ordinal)) { | ||
| int slash = javaName.IndexOf ('/'); | ||
| if (slash > 0) { | ||
| return "crc64.../" + javaName.Substring (slash + 1); | ||
| return "hash.../" + javaName.Substring (slash + 1); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We prefixed |
||
| } | ||
| } | ||
| return javaName; | ||
|
|
||
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.
@copilot I think we should drop xxhash64, it doesn't bring any value after all. Let's keep the compat crc64 + the new "fast" crc64
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.
Done in fa8493d. I removed the trimmable
XxHash64path and now keep only legacyLowercaseCrc64+ fastCrc64. No UI changes (screenshot N/A).