[TrimmableTypeMap] Make adjustments to the base JniValueManager and JniTypeManager to make implementing the trimmable type map easier#1454
Open
simonrozsival wants to merge 20 commits into
Conversation
The runtime hot path uses GetTypeSignature(), and GetTypeSignatures() is no longer needed for this helper assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the object proxy value marshaler under JniValueManager so non-reflection value managers can reuse JavaProxyObject marshaling without duplicating the managed proxy type. Expose the existing object and peerable marshalers to derived value managers, and use ConditionalWeakTable.GetValue for JavaProxyObject caching. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors proxy/peerable value marshaling so it can be reused by JniValueManager implementations which don’t inherit from ReflectionJniValueManager, enabling external value managers (e.g., dotnet/android trimmable type-map managers) to reuse Java.Interop’s proxy/peerable behavior.
Changes:
- Moves the proxy
objectvalue marshaler into theJniValueManagerbase (and exposes proxy/peerable marshalers to derived managers). - Updates proxy caching to use
ConditionalWeakTable.GetValue()inJavaProxyObject. - Adjusts type-manager tests to use singular type-signature lookup and updates public API tracking.
Show a summary per file
| File | Description |
|---|---|
| tests/Java.Interop-Tests/Java.Interop/JniTypeManagerTests.cs | Simplifies assertions to use GetTypeSignature() only. |
| src/Java.Interop/PublicAPI.Unshipped.txt | Tracks newly exposed marshalers on JniValueManager. |
| src/Java.Interop/Java.Interop/JniRuntime.ReflectionJniValueManager.cs | Switches fallback marshaler to the base ObjectValueMarshaler. |
| src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs | Introduces reusable proxy marshaler and exposes proxy/peerable marshalers to derived managers. |
| src/Java.Interop/Java.Interop/JniBuiltinMarshalers.cs | Updates built-in marshaler mapping for JavaProxyObject to use the shared marshaler. |
| src/Java.Interop/Java.Interop/JavaProxyObject.cs | Uses ConditionalWeakTable.GetValue() for proxy caching. |
Copilot's findings
- Files reviewed: 6/6 changed files
- Comments generated: 2
Expose primitive array type signature and value marshaler lookup from the base managers so non-reflection value managers can reuse the full Java.Interop primitive array marshaler set. Mark expression-based value marshaler contract tests as unsupported for trimmable typemap consumers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep JavaProxyObject cache access locked while using ConditionalWeakTable.GetValue. Make non-generic DestroyArgumentState tolerate null values for value-type marshalers, and mark Java.Interop tests that exercise expression-based marshaling, ManagedPeer, or legacy native registration as unsupported for trimmable typemap consumers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
|
/review |
|
✅ Java.Interop PR Reviewer completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Needs Changes
The refactoring to make proxy/peerable value marshalers reusable from the base JniValueManager is well-structured. The DestroyArgumentState null-handling fix and ConditionalWeakTable.GetValue() simplification are nice improvements.
Issues
| Severity | Count | Summary |
|---|---|---|
| ❌ Error | 1 | T4 templates out of sync with generated files |
| 1 | Test coverage reduction without justification | |
| 💡 Suggestion | 2 | Redundant lock, readonly singleton + stale suppression |
The blocking issue is that JavaPrimitiveArrays.tt and JniBuiltinMarshalers.tt were not updated to match the changes to their generated .cs files. Regenerating from templates will revert the code changes.
Positive callouts
- Clean separation of marshaler logic into the base class for reusability
- Good use of
ConditionalWeakTable.GetValue()to simplify caching - The
DestroyArgumentStatenull-safety fix properly handles value types - Public API surface (
PublicAPI.Unshipped.txt) is correctly maintained [Category ("TrimmableTypeMapUnsupported")]annotations are appropriate for tests that depend on reflection-based managers
Generated by Java.Interop PR Reviewer for issue #1454 · ● 6.8M
These tests rely on desktop ManagedPeer construction/registration paths that the Android trimmable typemap intentionally does not support. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep value marshaler helper classes as top-level internals and remove the primitive-array/type-manager helper API churn from the Java.Interop branch. Android keeps the trimmable-only primitive array handling locally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Give CrossReferenceBridge explicit JniPeerMembers so Android can construct the intended Java peer without relying on the desktop ManagedPeer constructor path, and categorize the test as GCBridge instead of unsupported. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Android trimmable lane supports method replacement; keep ReplacementTypeUsedForMethodLookup active by categorizing it as Remapping instead of TrimmableTypeMapUnsupported. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Android trimmable lane supports this dispatch scenario once the ManagedPeer-dependent Java fixtures are replaced, so categorize MethodBindingTests as MethodBinding instead of unsupported. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove the Android-trimmable fixture workarounds from the Java.Interop branch and leave ManagedPeer-dependent tests categorized as TrimmableTypeMapUnsupported. Restore JniValueMarshaler and JniTypeManagerTests changes so this PR stays focused. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Let JavaObjectArray request value marshaler state directly from JniValueManager. ReflectionJniValueManager keeps using value marshalers internally, while trimmable Android can override the abstract state APIs without exposing value marshalers on that path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
changed the title
Member
Author
|
/review |
|
✅ Java.Interop PR Reviewer completed successfully! |
There was a problem hiding this comment.
✅ LGTM — Clean, well-structured abstraction layer
The PR correctly lifts JavaObjectArray's marshaler state operations from JniValueMarshaler<T> direct calls up to JniValueManager virtual dispatch, enabling trimmable value managers to implement these paths without reflection. The architecture follows established patterns in this codebase.
What looks good
- Clean separation: The
JniValueManagerbase class provides the internal API + abstract core, andReflectionJniValueManagerdelegates to the existing marshaler infrastructure — exactly the pattern used for other abstract methods. - No in-repo breakage: All
JniValueManagersubclasses in this repo derive fromReflectionJniValueManagerand inherit the new implementations. - Public API surface:
PublicAPI.Unshipped.txtentries are complete and consistent. - Test categorization: The
TrimmableTypeMapUnsupportedcategory correctly identifies ManagedPeer-dependent tests for downstream exclusion. GetTypeNamefix: Switching from.NametoGetTypeName()for the marshaler type in the test expression string correctly handles generic type names.
Minor suggestions (3 × 💡)
| # | Concern |
|---|---|
| 1 | CreateValueMarshalerState defaults synchronize to ParameterAttributes.In while the underlying marshalers default to 0 — subtle behavioral shift in Clear(). |
| 2 | ObjectValueMarshaler/PeerableValueMarshaler properties lack doc comments explaining their intended downstream use. |
| 3 | Non-generic Type-based overloads have no test coverage within this repo. |
CI
Only the CLA check is visible; no build/test CI results found on the PR. The code changes are straightforward delegation — unlikely to break existing behavior — but downstream validation (dotnet/android integration branch) is the key verification gate per the PR description.
Generated by Java.Interop PR Reviewer for issue #1454 · ● 7.1M
Keep JavaObjectArray object-reference marshaling on JniValueManager while removing unused state overloads, default-value state APIs, and exposed proxy marshaler accessors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace object-array marshaler state cleanup with a direct JniObjectReference result. The reflection manager now uses the existing marshaler to create and destroy temporary state internally before returning an independent local reference. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename the JavaObjectArray helper hook to CreateLocalObjectReferenceArgument so callers can see that the returned JNI reference is owned and must be disposed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
added
the
ready-to-review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1441.
Prerequisite for dotnet/android#11617.
This keeps the Java.Interop changes focused on the small base hook dotnet/android needs for the trimmable type-map integration. The reflection value manager continues to use value marshalers internally, while Android's generated/trimmable value manager can provide the only production
JavaObjectArray<T>element-assignment object-reference path without implementingGetValueMarshaler*().Changes
JniValueManagerobject-reference API forJavaObjectArray<T>.SetElementAt():CreateLocalObjectReferenceArgument(Type type, object? value)returns an owned localJniObjectReferencefor element assignment. Callers must dispose the returned reference.ReflectionJniValueManagerimplementation detail: reflection creates marshaler state, copies out an independent local reference, then destroys the state immediately.JavaObjectArray<T>to call the value manager directly instead of callingGetValueMarshaler<T>()in production paths.JavaObjectArray<T>.Clear()to set array slots to Java null directly; it no longer needs value-manager or value-marshaler state.ParameterAttributes synchronizefrom this value-manager object-reference path.Non-goals
GetValueMarshaler*().Validation
dotnet build external/Java.Interop/src/Java.Interop/Java.Interop.csproj -p:Configuration=Debug -m:1 -nodeReuse:false --no-restore -v:minimaldotnet test tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests/Microsoft.Android.Sdk.TrimmableTypeMap.Tests.csproj -v minimal --no-restore(562passed)dotnet build src/Mono.Android/Mono.Android.csproj -p:Configuration=Debug -p:AndroidSdkDirectory=/Users/simonrozsival/android-toolchain/sdk -m:1 -nodeReuse:false --no-restore -v:minimalcompiledMono.Android.Runtime.dll; the remaining local failure is Android SDK provisioning (extras/android/m2repository.staginganddocs.stagingmissing), not C# or trim-analyzer errors.