Skip to content

[CoreCLR] java to managed typemap #10075

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

grendello
Copy link
Contributor

@grendello grendello commented Apr 24, 2025

This is a follow-up to #10065 (which must be merged first)

Implement an optimized (hash-based) managed-to-java typemap lookup for Debug builds, with a
fallback to string-based lookups if hash clashes are detected.

Additionally, optimize usage of certain sets of strings in native code, namely type and assembly names. Instead of storing them as "standard" string pointers, they are now contained in "blobs" - arrays of character data, separated with NUL characters. This approach gets rid of a potentially large number of native code relocations (each string is a pointer), replacing X pointers with a single pointer + offset when accessing a string. It also has a
side effect of (slightly) reducing generated code size.

@grendello grendello force-pushed the dev/grendel/clr-host-java-to-managed-typemap branch 2 times, most recently from 03a87c4 to ecedf89 Compare April 28, 2025 15:21
@grendello grendello force-pushed the dev/grendel/clr-host-java-to-managed-typemap branch from ecedf89 to bac4183 Compare April 29, 2025 09:14
@grendello grendello changed the title [CLR] java to managed typemap [CoreCLR] java to managed typemap Apr 29, 2025
@grendello grendello marked this pull request as ready for review April 29, 2025 09:48
@grendello grendello requested a review from Copilot April 29, 2025 09:49
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements an optimized, hash‐based managed-to‐java typemap lookup for Debug builds while also optimizing the storage of type and assembly names by switching from pointer arrays to string blobs. Key updates include:

  • Reordering of timing/logging checks in the native runtime initialization.
  • Adoption of templated hash functions and updated binary search operations.
  • Significant changes to TypeMap structures and associated C# generators for improved efficiency and reduced relocations.

Reviewed Changes

Copilot reviewed 19 out of 21 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/native/common/runtime-base/timing-internal.cc Reordered the immediate_logging check to restrict option processing when immediate logging is enabled.
src/native/common/include/shared/xxhash.hh Replaced a char*-based hash function with a templated version for broader applicability.
src/native/clr/include/xamarin-app.hh Updated TypeMapEntry and related structures to use offset-based fields and std::string_view for string blobs.
src/native/clr/host/typemap.cc Modified typemap lookup functions (debug and release) to support hash-based and string-based matching.
src/Xamarin.Android.Build.Tasks/Utilities/*.cs Revised type mapping generators to generate and reference additional string length and offset fields.
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/DeviceTest.cs Updated test utilities to support new parameters for environment variable injection.
Files not reviewed (2)
  • src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets: Language not supported
  • src/native/native.targets: Language not supported
Comments suppressed due to low confidence (4)

src/native/common/include/shared/xxhash.hh:166

  • Ensure that the updated templated hash function is compatible with existing code that depends on hash(const char*, size_t), particularly regarding type inference.
template<typename T> [[gnu::always_inline]] static auto hash (const T *p, size_t len) noexcept -> XXH64_hash_t

src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/DeviceTest.cs:211

  • [nitpick] Ensure that the new 'environmentVariables' parameter is adequately tested across different project run scenarios to confirm expected behavior.
protected static void RunProjectAndAssert (XamarinAndroidApplicationProject proj, ProjectBuilder builder, string logName = "run.log", Dictionary<string, string> environmentVariables = null, bool doNotCleanupOnUpdate = false, string [] parameters = null)

src/native/common/runtime-base/timing-internal.cc:30

  • The reordering of the 'if (immediate_logging)' check may change the timing of when immediate logging is bypassed; please verify that this modification maintains the intended behavior.
if (immediate_logging) {

src/native/clr/include/xamarin-app.hh:66

  • Changing 'from' from a pointer to a uint32_t offset requires thorough validation against all consumers of TypeMapEntry to avoid misinterpretation of data.
const uint32_t from;

@grendello
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant