Skip to content

[NativeAOT] Use indexed temporary peers in GC bridge#12145

Open
simonrozsival wants to merge 3 commits into
mainfrom
dev/simonrozsival/nativeaot-temporary-peer-map
Open

[NativeAOT] Use indexed temporary peers in GC bridge#12145
simonrozsival wants to merge 3 commits into
mainfrom
dev/simonrozsival/nativeaot-temporary-peer-map

Conversation

@simonrozsival

@simonrozsival simonrozsival commented Jul 17, 2026

Copy link
Copy Markdown
Member

Summary

Replace the shared CoreCLR/NativeAOT GC bridge's temporary-peer std::unordered_map with an allocation-backed TemporaryPeerMap.

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 temporary mono.android.GCUserPeer solely 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.hh
  • src/native/clr/host/bridge-processing.cc

TemporaryPeerMap lifetime

  1. The constructor scans all SCCs, rejects pre-existing marker values, and counts exactly how many temporary peers are required.
  2. If none are required, it performs no allocation.
  3. Otherwise it reserves JNI local-reference capacity for all temporary peers plus slack, then allocates one zero-initialized jobject array with calloc.
  4. add() creates the temporary GCUserPeer, stores it in the next array slot, and writes the encoded slot index into the SCC's Count field.
  5. Cross-reference target selection detects the encoded marker and retrieves the peer directly from the array.
  6. At the end of the scoped cross-reference pass, the destructor deletes every temporary JNI local reference, resets every marked SCC to Count == 0, frees the array, and clears its bookkeeping.
  7. Normal weak-global-reference processing starts only after the destructor has restored the original SCC shape.

Index encoding

Count is unsigned, so the temporary index is stored as ~index, which has the same bit pattern as -(index + 1):

  • index zero remains representable;
  • the high bit acts as the temporary-peer marker;
  • encoding rejects indexes that already use the marker bit;
  • decoding verifies the marker and bounds-checks the resulting array index;
  • the constructor verifies that runtime-provided SCC counts do not already use the reserved marker space.

JNI initialization and safety

  • cache the mono.android.GCUserPeer class and constructor during runtime initialization;
  • preserve the existing cached mono.android.IGCUserPeer method IDs used for reference callbacks;
  • reserve local-reference capacity before creating a potentially large temporary-peer set;
  • clear and log an EnsureLocalCapacity failure consistently with the previous implementation;
  • fail fast on allocation failure, peer-construction failure, invalid markers, capacity overruns, missing peers, and out-of-range indexes;
  • preserve existing fail-fast handling for Java exceptions raised by monodroidAddReference or monodroidClearReferences;
  • explicitly delete copy and move construction/assignment so the owning array and JNI local references cannot be shallow-copied.

Behavior preserved

  • temporary peers remain alive until every cross-SCC reference has been added;
  • temporary local references are released before the Java GC is triggered;
  • zero-, one-, and multi-peer SCC handling remains unchanged;
  • cross-reference source/destination selection and refs_added bookkeeping remain unchanged;
  • the runtime receives its SCC array back with all temporary markers removed;
  • CoreCLR and NativeAOT continue to use the same shared bridge implementation and host-specific peer callback hooks.

Scope and non-goals

  • This PR changes only temporary-peer storage; it does not change the GC bridge graph algorithm or collection policy.
  • It does not change Java peer APIs, reference callback names, or GC trigger behavior.
  • It does not introduce robin-map or another replacement hash table.
  • The unrelated logging/path/source-location ownership cleanup remains in [NativeAOT] Reduce owning C++ standard library state #12142.

Expected impact

  • remove std::unordered_map from shared CoreCLR/NativeAOT temporary-peer processing;
  • remove the associated std::__ndk1::__next_prime and hash-table allocation/code roots;
  • replace per-node hash-table bookkeeping with one exact-size array allocation;
  • make temporary JNI reference ownership and SCC marker restoration explicit through RAII;
  • preserve GC bridge semantics while reducing C++ standard-library reachability.

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 --check passed;
  • the ownership hardening is declaration-only and introduces no runtime code;
  • CI is validating head d4315727e.

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
Copilot AI review requested due to automatic review settings July 17, 2026 07:22
simonrozsival added a commit that referenced this pull request Jul 17, 2026
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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 TemporaryPeerMap to pre-count SCCs needing temporary peers, allocate an exact-size jobject array, and encode the peer index into StronglyConnectedComponent.Count during 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 owning std::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
@simonrozsival
simonrozsival enabled auto-merge (squash) July 17, 2026 20:20
@simonrozsival

Copy link
Copy Markdown
Member Author

/review

@simonrozsival simonrozsival added the ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable). label Jul 17, 2026
@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Android PR Reviewer completed successfully!

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ 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 ~index encoding preserves index 0, uses the sign bit as the marker, and both encode/decode guard against markerless/marker-set inputs with abort_unless. get() bounds-checks against count. ✅
  • RAII lifetime is correct: local refs are released and every marked SCC Count is reset to 0 in the destructor before prepare_for_java_collection runs the weak-global-ref pass, so scc.Count is 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

Comment thread src/native/clr/host/bridge-processing.cc
Comment thread src/native/clr/host/bridge-processing.cc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drop-libcpp Work to remove the libc++ dependency from Android NativeAOT ready-to-review This PR is ready to review/merge, I think any CI failures are just flaky (ignorable).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants