[TrimmableTypeMap] Use XxHash64 package naming by default with LowercaseCrc64 compatibility#11193
[TrimmableTypeMap] Use XxHash64 package naming by default with LowercaseCrc64 compatibility#11193
Conversation
Agent-Logs-Url: https://github.com/dotnet/android/sessions/425dd1fe-e6a4-43e0-b0a9-de6b1e343c11 Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/android/sessions/425dd1fe-e6a4-43e0-b0a9-de6b1e343c11 Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
|
@copilot I would prefer changing the prefix from |
Agent-Logs-Url: https://github.com/dotnet/android/sessions/b04a9e6a-bf80-4ff1-8601-4078c145728e Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
There was a problem hiding this comment.
Note that there is the property:
<AndroidPackageNamingPolicy>LowercaseCrc64</AndroidPackageNamingPolicy>We will probably want to introduce a new one for XxHash64.
It is a slight breaking change, because people could have a hardcoded package name in a script for launching their main activity, etc.
So, I think what we did in some really old Xamarin.Android release, we changed the default but allowed projects to still use the old one.
So funny thing is that the CSC64 we have in our C++(or C#?) code and CSC64 that is in System.IO.Hashing produce different results. I decided to use the System.IO.Hashing one in the trimmable typemap so that would be a breaking change anyway. I will need to revisit this and try to make the |
|
@copilot let's respect |
…efault Agent-Logs-Url: https://github.com/dotnet/android/sessions/a90e2abd-ca59-4b98-a498-64caa983b1d0 Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Implemented in ec92067: trimmable typemap now respects
... Addressed in ec92067: added trimmable support for |
| var data = System.Text.Encoding.UTF8.GetBytes (value); | ||
| var hash = System.IO.Hashing.XxHash64.Hash (data); | ||
| return BitConverter.ToString (hash).Replace ("-", "").ToLowerInvariant (); |
There was a problem hiding this comment.
@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 ToHexString method which is a lot more efficient than what we have right now. Also I don't think we need to hash UTF-8, we can simply do ReadOnlySpan<byte> utf16Bytes = MemoryMarshal.AsBytes(text.AsSpan()); , right? We hash a lot of strings during the build process, so let's make sure we don't unnecessarily slow down builds.
There was a problem hiding this comment.
Implemented in 8d5709b: reduced allocations in JavaPeerScanner by switching to span-based hex formatting (with stackalloc), using non-allocating XxHash64.Hash(ReadOnlySpan<byte>, Span<byte>), and removing intermediate "{ns}:{assembly}" string allocations via pooled UTF-8 buffers. I kept UTF-8 input for hash compatibility with existing outputs/policies.
Agent-Logs-Url: https://github.com/dotnet/android/sessions/9fd6ed14-5c7b-463e-b1b0-ebefd496f067 Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
simonrozsival
left a comment
There was a problem hiding this comment.
@copilot address these comments
I want to move the hashing code from JavaPeerScanner to a separate static ScannerHashingHelper class which can be better tested. I want a test that will just simply validate some known inputs to some known outputs of the legacy crc64 algorithm to prove we're maintaining backwards compatibility.
| <AndroidVersionCodePattern Condition=" '$(AndroidUseLegacyVersionCode)' != 'True' And '$(AndroidVersionCodePattern)' == '' ">{abi}{versionCode:D5}</AndroidVersionCodePattern> | ||
| <AndroidResourceGeneratorTargetName>UpdateGeneratedFiles</AndroidResourceGeneratorTargetName> | ||
| <AndroidUseApkSigner Condition=" '$(AndroidUseApkSigner)' == '' ">True</AndroidUseApkSigner> | ||
| <AndroidPackageNamingPolicy Condition=" '$(AndroidPackageNamingPolicy)' == '' ">LowercaseCrc64</AndroidPackageNamingPolicy> |
There was a problem hiding this comment.
the XxHash64 should be the default just for the trimmable typemap
There was a problem hiding this comment.
Addressed in 05cebc7: reverted the global AndroidPackageNamingPolicy default and scoped XxHash64 as the default specifically for trimmable typemap generation.
Agent-Logs-Url: https://github.com/dotnet/android/sessions/144010de-77de-456e-aee5-4db9e9a1a5cb Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Implemented in 05cebc7: moved scanner hashing into
Handled in 05cebc7: trimmable typemap continues to respect explicit
|
This updates trimmable type map generation to default to
XxHash64-based package naming for unregistered types, while preserving backwards compatibility for projects that explicitly set:Package naming policy support in trimmable typemap
AndroidPackageNamingPolicyinto trimmable typemap generation.XxHash64(trimmable default) →xx64...LowercaseCrc64→crc64...using the existing legacy/custom CRC64 implementation (same style used in existing typemap paths).Mono.Androidnamespace-preserving behavior unchanged.Default policy update
XxHash64is now the default only for trimmable typemap generation.AndroidPackageNamingPolicydefault remainsLowercaseCrc64for non-trimmable typemap paths.Hashing implementation refactor
JavaPeerScannerinto a dedicated staticScannerHashingHelperfor easier focused testing.Targeted test updates
xx64defaults.LowercaseCrc64produces legacycrc64names and differs from defaultXxHash64output.Related plumbing updates
TrimmableTypeMapGeneratorand scanner hashing logic.