[NativeAOT] Use indexed temporary peers in GC bridge#12145
[NativeAOT] Use indexed temporary peers in GC bridge#12145simonrozsival wants to merge 3 commits into
Conversation
Replace the shared GC bridge unordered map with the negative-index TemporaryPeerMap approach from #11311. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70f63eb7-6599-414c-a947-d860705aa0fa
Move the TemporaryPeerMap changes to #12145 so this branch only contains the remaining owning-state cleanup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70f63eb7-6599-414c-a947-d860705aa0fa
There was a problem hiding this comment.
Pull request overview
Updates the shared CoreCLR/NativeAOT GC bridge cross-reference pass to replace the temporary-peer std::unordered_map with an indexed, exact-capacity array-backed TemporaryPeerMap, reducing STL/hash-map dependencies while keeping scoped, deterministic lifetime management for temporary JNI peers.
Changes:
- Introduces
TemporaryPeerMapto pre-count SCCs needing temporary peers, allocate an exact-sizejobjectarray, and encode the peer index intoStronglyConnectedComponent.Countduring cross-reference processing. - Switches temporary-peer ownership/cleanup to RAII (deleting local refs, freeing the array, and restoring SCC counts at scope exit).
- Refactors bridge processing helpers to thread
TemporaryPeerMap&through SCC preparation and cross-reference edge handling, removing the owningstd::unordered_map.
Show a summary per file
| File | Description |
|---|---|
| src/native/clr/include/host/bridge-processing-shared.hh | Adds TemporaryPeerMap API and updates bridge-processing method signatures to use it instead of std::unordered_map. |
| src/native/clr/host/bridge-processing.cc | Implements TemporaryPeerMap (allocation, index encoding/decoding, RAII cleanup) and rewires SCC + cross-reference processing to use the new map. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
Prevent accidental shallow copies from duplicating ownership of the JNI local references and calloc buffer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70f63eb7-6599-414c-a947-d860705aa0fa
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
⚠️ Review summary — clean refactor, no blocking issues found
Solid, well-scoped change: replaces the std::unordered_map temporary-peer storage with an allocation-backed TemporaryPeerMap that encodes the peer-array index into the SCC Count field. I reviewed the full bridge-processing.cc/.hh and reconciled it against the PR description.
Correctness — verified:
- The
~indexencoding preserves index 0, uses the sign bit as the marker, and bothencode/decodeguard against markerless/marker-set inputs withabort_unless.get()bounds-checks againstcount. ✅ - RAII lifetime is correct: local refs are released and every marked SCC
Countis reset to 0 in the destructor beforeprepare_for_java_collectionruns the weak-global-ref pass, soscc.Countis safe to use as a real count again. Ordering matches the previous implementation. ✅ - Copy/move ctor + assignment are
= deleted, so the owning array and JNI local refs can't be shallow-copied (Postmortem#17). ✅ - Constructor defensively rejects runtime-provided counts that already use the marker bit, reserves local-ref capacity with the same clamp/log behavior as before, and no-allocs when zero temporary peers are needed. ✅
- Uses
<cstdlib>,static_cast, and RAII consistent with the native rules.
Behavior preserved: zero/one/multi-peer SCC handling, cross-reference source/dest selection, refs_added bookkeeping, and the shared CoreCLR/NativeAOT hooks are unchanged. The runtime gets its SCC array back with all markers cleared.
Notes (non-blocking):
- No new automated tests — acceptable here since this is an internal native refactor exercised by existing GC bridge device tests, and semantics are unchanged. Worth confirming the GC bridge device suite passes on both CoreCLR and NativeAOT.
- Two 💡 inline suggestions posted (RAII
unique_ptr, parameter naming). - CI: no completed statuses reported yet on head
229806e(validation pending per the PR description) — not verified green.
Nice work reducing std:: reachability while keeping the semantics intact.
Generated by Android PR Reviewer for #12145 · 89.9 AIC · ⌖ 13.2 AIC · ⊞ 6.8K
Comment /review to run again
Summary
Replace the shared CoreCLR/NativeAOT GC bridge's temporary-peer
std::unordered_mapwith an allocation-backedTemporaryPeerMap.The design follows the approach previously developed in #11311: an empty strongly connected component temporarily carries an encoded peer-array index in
StronglyConnectedComponent.Count, so the bridge does not need a general-purpose C++ hash table during its scoped cross-reference pass.Split from #12142. The two PRs are independent and can merge in either order. Part of #12139.
Background
During GC bridge processing, each strongly connected component (SCC) must behave like one Java object:
Count == 1: the existing Java peer represents the SCC directly;Count > 1: the bridge adds circular references so all peers remain alive or are collected together;Count == 0: there is no Java peer, so the bridge creates a temporarymono.android.GCUserPeersolely to represent that SCC while cross-SCC references are established.The previous implementation stored those temporary peers in
std::unordered_map<size_t, jobject>, keyed by SCC index. The required key set and capacity are already known before processing begins, and lookup is only needed within one short scope, making a hash table unnecessary.Implementation
Files:
src/native/clr/include/host/bridge-processing-shared.hhsrc/native/clr/host/bridge-processing.ccTemporaryPeerMaplifetimejobjectarray withcalloc.add()creates the temporaryGCUserPeer, stores it in the next array slot, and writes the encoded slot index into the SCC'sCountfield.Count == 0, frees the array, and clears its bookkeeping.Index encoding
Countis unsigned, so the temporary index is stored as~index, which has the same bit pattern as-(index + 1):JNI initialization and safety
mono.android.GCUserPeerclass and constructor during runtime initialization;mono.android.IGCUserPeermethod IDs used for reference callbacks;EnsureLocalCapacityfailure consistently with the previous implementation;monodroidAddReferenceormonodroidClearReferences;Behavior preserved
refs_addedbookkeeping remain unchanged;Scope and non-goals
Expected impact
std::unordered_mapfrom shared CoreCLR/NativeAOT temporary-peer processing;std::__ndk1::__next_primeand hash-table allocation/code roots;Validation
The implementation is the isolated GC bridge change previously carried in #12142, plus explicit non-copyable/non-movable ownership semantics for
TemporaryPeerMap.git diff --checkpassed;d4315727e.