[Native] Remove unused field weak_handle#12239
Conversation
This field on `JniObjectReferenceControlBlock` was never written to. The only time it was read was for debug logging. This shrinks the size of the control block from 32 to 16 bytes on 64-bit platforms. While I was at it, I added the the `StructLayout` attribute to the `JniObjectReferenceControlBlock` in C#. This makes the warning suppression for CS0649 no longer necessary.
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
🤖 Review — ✅ LGTM (minor)
Clean, well-scoped removal of the unused weak_handle field from JniObjectReferenceControlBlock. Verified independently before reading the description.
Verification performed:
- 🔎
weak_handlehas zero remaining readers or writers anywhere in the tree — confirmed via full-repo search. - 🧱 All four struct definitions are updated in lockstep, keeping the field layout identical across the managed/native boundary:
external/Java.Interop/.../JniObjectReferenceControlBlock.cssrc/Mono.Android/.../JavaMarshalRegisteredPeers.cs(internal mirror)src/native/clr/include/host/gc-bridge.hh(CoreCLR)src/native/mono/monodroid/osbridge.hh/osbridge.cc(Mono)
- 📐 Resulting layout is
handle(8) +handle_type(4) +refs_added(4) = 16 bytes on 64-bit, matching the PR's claim.bridge-processing.cconly toucheshandle/handle_type/refs_added, so the CLR bridge is unaffected. - 🪵 The two
osbridge.ccdiagnostic log sites that referencedkey_handle(derived solely fromweak_handle) are correctly cleaned up, including the format strings — no dangling format specifiers. - ✅ Dropping the
#pragma warning disable CS0649is now valid since the mirror struct is explicitly assigned/read via native memory and the previously-unassigned field is gone.
Notes:
- 💡 One optional consistency suggestion posted inline re: adding the explicit
[StructLayout]to the sourceJava.Interopstruct as well. - PR is a draft and
mergeable_stateisblocked; the description leaves "Unit tests" unchecked. This is a pure field/logging removal with no behavioral change, so a dedicated test isn't strictly warranted — but confirm the native GC-bridge builds (Mono + CoreCLR) pass in CI before merging, since this touches the managed↔native struct contract.
Severity counts — ❌ 0 ·
Generated by Android PR Reviewer for #12239 · 64.4 AIC · ⌖ 18.6 AIC · ⊞ 6.8K
Comment /review to run again
|
|
||
| #pragma warning disable CS0649 // Field 'JavaMarshalRegisteredPeers.HandleContext.JniObjectReferenceControlBlock.*' is never assigned to, and will always have its default value 0 | ||
| // This is an internal mirror of the Java.Interop.JniObjectReferenceControlBlock | ||
| [StructLayout (LayoutKind.Sequential)] |
There was a problem hiding this comment.
🤖 💡 Consistency — Good call adding [StructLayout (LayoutKind.Sequential)] here to make the native-layout dependency explicit and drop the CS0649 suppression. For symmetry, consider adding the same attribute to the source Java.Interop.JniObjectReferenceControlBlock struct, since the native C++ definitions (gc-bridge.hh, osbridge.hh) read both by field offset. C# defaults structs to sequential layout so this isn't a bug today, but making it explicit on both mirrors documents that the ordering is intentional and load-bearing.
(Rule: Interop struct layout must match native definitions)
There was a problem hiding this comment.
I tried to keep these changes as minimal as possible, since .NET 11 is probably branching soon. Also, the definition of Java.Interop.JniObjectReferenceControlBlock was recently merged into this repository from the old Java.Interop repo. I'm assuming that duplicate copy is on the way out.
Anyways, I was not sure where to draw the line on these changes. Let me know if you want any revisions.
This field on
JniObjectReferenceControlBlockwas never written to. The only time it was read was for debug logging.This shrinks the size of the control block from 32 to 16 bytes on 64-bit platforms.
While I was at it, I added the the
StructLayoutattribute to theJniObjectReferenceControlBlockin C#. This makes the warning suppression for CS0649 no longer necessary.