[NativeAOT] Use indexed temporary peers in GC bridge#12145
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 Continue the printf-style logging migration introduced by #12140 in the shared native timing infrastructure. The timing headers are used by MonoVM, CoreCLR, and NativeAOT. All three runtimes now use the same printf-style path for variable timing diagnostics rather than maintaining MonoVM-only `std::format` branches. **Base/dependency:** #12140 must merge first. This PR targets `dev/simonrozsival/nativeaot-printf-logging` so its diff contains only the timing migration. Part of #12139. ## Scope Only two shared timing headers change: - `src/native/common/include/runtime-base/timing.hh` - `src/native/common/include/runtime-base/timing-internal.hh` These files do not overlap #12141, #12142, #12145, #12148, #12150, or #12155. ## Changes ### Managed timing records - replace the owning `std::format` result in `Timing::do_log()` with `log_writef()` for every runtime; - preserve the exact `message; elapsed: seconds:milliseconds::nanoseconds` field order; - pass duration values as explicitly converted `unsigned long long` values matching `%llu`. ### Fast timing diagnostics Migrate variable diagnostics to `log_warnf()` for every runtime: - timing event buffer reallocation sizes; - `CLOCK_MONOTONIC_RAW` errors; - unknown event-kind values; - invalid event-index source method names. Constant warning messages continue using the existing non-formatting `string_view` overload. ## Behavior preserved - timing enablement and category gating are unchanged; - timing sequence acquisition/release is unchanged; - elapsed duration units, values, and output order are unchanged; - event-buffer growth, clock error handling, unknown-event fallback, and index validation are unchanged; - no timing data structures, locks, vectors, strings, or event lifecycle code change. ## Runtime behavior | Runtime | Result | |---|---| | NativeAOT | Uses #12140 printf helpers for variable timing diagnostics | | CoreCLR | Uses #12140 printf helpers for variable timing diagnostics | | MonoVM | Uses the matching MonoVM printf helper implementation from #12140 | ## Measured impact Representative Android arm64 Release objects compiled before the MonoVM path was unified: | Runtime/object | Before | After | Difference | |---|---:|---:|---:| | CoreCLR `internal-pinvokes-clr.cc.o` | 145,472 B | 17,080 B | -128,392 B (-88.26%) | | NativeAOT `host.cc.o` | 176,104 B | 175,960 B | -144 B | CoreCLR's representative object drops from 58 formatting symbols to zero. NativeAOT's representative object still contains formatting symbols from other included functionality, but the timing call sites migrated here no longer instantiate them. ## Non-goals - no new timing tests or logging test framework; - no change to timing data ownership or synchronization; - no changes to the logging helper implementation introduced by #12140; - no attempt to migrate unrelated timing output/file-generation code outside these shared timing headers. ## Validation - `git diff --check`; - NDK Clang C++23 syntax compilation of all 36 Android arm64 Release NativeAOT source variants; - NDK Clang C++23 syntax compilation of all 38 Android arm64 Release CoreCLR source variants; - NDK Clang C++23 syntax compilation of all 31 Android arm64 Release MonoVM source variants; - representative before/after object compilation, size comparison, and symbol inspection; - focused review of chrono values, integer format widths, source-location output, and category semantics; - latest head: `21ff66b26`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Continue the printf-style logging migration introduced by #12140 by converting shared CoreCLR/NativeAOT JNI reference logging in `OSBridge`. The `src/native/clr/` location is shared infrastructure: the NativeAOT host explicitly compiles `clr/host/os-bridge.cc`, while MonoVM uses its separate `mono/monodroid/osbridge.cc` implementation. This PR therefore affects CoreCLR and NativeAOT, but not MonoVM. **Base/dependency:** #12140 must merge first. This PR targets `dev/simonrozsival/nativeaot-printf-logging` so its diff contains only this migration. Part of #12139. ## Scope Only two files change: - `src/native/clr/host/os-bridge.cc` - `src/native/clr/include/host/os-bridge.hh` This does not overlap the open sibling work: - #12141 changes `gc-bridge.cc/.hh` — worker-thread and semaphore handoff; - #12142 changes logging/path ownership and source-location parsing elsewhere; - #12145 changes `bridge-processing.cc/.hh` — temporary-peer graph storage; - #12148 changes shared host/runtime utility logging. ## Changes ### Format reference records once - add a private printf-checked `OSBridge::log_itf()` helper; - use `vasprintf()` to produce one NUL-terminated reference-log line; - pass the same formatted line to the existing logcat and file-writing path; - release the C allocation after both destinations have consumed it; - on allocation failure, fall back to the static format string rather than throwing from a `noexcept` `std::format` path. ### Migrate JNI reference events Replace six owning `std::format` constructions: - new global reference; - deleted global reference; - new weak global reference; - deleted weak global reference; - new local reference; - deleted local reference. The replacements use `%d`, `%p`, `%c`, and `%s` with compiler format checking from the private helper declaration. ### Remove ranges-heavy stack-trace splitting - replace `std::views::split` with an in-place `std::string_view` newline scan; - preserve empty, consecutive, and trailing lines; - preserve logcat-only, file-only, and combined output behavior; - write non-NUL-terminated line views with explicit lengths; - convert stack-trace and raw gref logcat forwarding to `%.*s` / `%s`. ## Responsibility boundary with #12141 - `GCBridge` (#12141) controls **when and where** a bridge round runs: callback handoff, worker thread, semaphore synchronization, and Java GC triggering. - `OSBridge` (this PR) tracks and logs **JNI reference events**: gref/weak-gref counters, reference creation/deletion lines, and optional stack traces. The classes call each other through existing stable helpers, but this PR changes no GC scheduling, graph processing, callback lifecycle, or synchronization state. ## Behavior preserved - reference counters are incremented/decremented at the same points; - logging remains gated by `LOG_GREF` / `LOG_LREF` before formatting; - each reference line is still emitted to logcat and, when configured, the corresponding file; - stack traces retain one output line per newline-delimited segment, including empty segments; - logcat/file flushing behavior is unchanged; - pointer, reference-type, thread-name, thread-id, and counter fields retain the same ordering and textual structure. ## Measured impact Android arm64 Release NativeAOT `os-bridge.cc` object, compiled with the repository-generated production NDK command: | Artifact | Before | After | Difference | |---|---:|---:|---:| | `os-bridge.cc.o` | 168,784 B | 31,288 B | -137,496 B (-81.46%) | | format/basic-string symbols | 21 | 6 | -15 | The remaining C++ string symbols come from other shared header functionality; the six `std::format` reference-log instantiations are removed. ## Non-goals - no new logging test framework is introduced; - no GC bridge synchronization or graph-processing code changes; - no MonoVM `osbridge.cc` changes; - no changes to the public logging helpers introduced by #12140. ## Validation - `git diff --check`; - no file overlap with #12141, #12142, #12145, or #12148; - NDK Clang C++23 syntax compilation of all 36 Android arm64 Release NativeAOT source variants; - NDK Clang C++23 syntax compilation of all 38 Android arm64 Release CoreCLR source variants; - before/after NativeAOT object compilation and symbol inspection; - focused read-only review of newline handling, allocation cleanup, OOM fallback, format types, category gating, and dual-destination output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Reduce owning C++ standard-library state reachable from the Android NativeAOT host. This PR is intentionally limited to three areas: 1. gref/lref logging path ownership; 2. NativeAOT-only Android path storage; 3. source-location function-name parsing used by native argument-validation diagnostics. The GC bridge temporary-peer cleanup was split into #12145. The two PRs are independent and can merge in either order. Part of #12139, which tracks removing the Android NativeAOT app's libc++ dependency. ## Motivation The NativeAOT host shares a significant amount of code with CoreCLR. Several shared helpers still introduced owning C++ standard-library objects or template instantiations into the NativeAOT build even though the NativeAOT path only needs bounded, process-lifetime data: - custom gref/lref log paths were stored in global `std::string` instances; - Android override/native-library paths were represented by shared owning containers intended primarily for CoreCLR behavior; - source-location formatting split compiler signatures through ranges and materialized `std::vector<std::string>` plus an owning result string. These are small individually, but each can root additional constructors, destructors, allocation helpers, exception machinery, or ranges/container implementation code in the final NativeAOT binary. ## Changes ### 1. Reference logging paths File: `src/native/clr/runtime-base/logger.cc` - replace global `std::string` instances for `gref_file` and `lref_file` with nullable C-string pointers; - add `set_log_file()` to perform overflow-checked allocation, copy the selected path, NUL-terminate it, and release the previous value; - treat an empty path as clearing the configured custom path; - expose parsed `gref=` / `lref=` values as `std::string_view` instead of returning a pointer into the parser without a length; - preserve the existing behavior where identical gref/lref paths reuse the same `FILE*`; - construct fallback paths such as `<override>/grefs.txt` in bounded `dynamic_local_string<SENSIBLE_PATH_MAX>` storage instead of an owning `std::string`. The observable logging behavior is unchanged: configured paths are tried first, fallback files remain under the override directory, and file-open failures continue to be logged by existing helpers. ### 2. NativeAOT Android path storage File: `src/native/clr/include/runtime-base/android-system.hh` Under `XA_HOST_NATIVEAOT`: - store `primary_override_dir` and `native_libraries_dir` in fixed process-lifetime buffers sized by `Constants::SENSIBLE_PATH_MAX`; - return C-string views of those buffers to NativeAOT callers; - construct the primary override path in bounded local storage, validate its length, then copy the complete NUL-terminated path into the static buffer; - compile out CoreCLR-oriented owning state that NativeAOT does not use, including the owning app-library/override-directory containers and debug bundled-property map; - keep CoreCLR's existing `std::string`, array, span, update-directory, and debug-property behavior unchanged. This is a compile-time host specialization, not a runtime behavior switch. ### 3. Source-location function-name parsing File: `src/native/common/include/shared/cpp-util.hh` - remove the ranges, vector, and owning-string implementation previously used by `get_function_name()`; - scan the compiler-provided signature in place through `std::string_view`; - locate the outer function parameter list while accounting for nested parentheses; - locate the relevant function component while accounting for template, array, and parenthesized nesting; - preserve readable handling of scoped functions, free operators, scoped operators, anonymous namespaces, lambdas, and templated signatures; - return a non-owning view and pass it to diagnostics with `%.*s`, avoiding construction of a temporary result string. Malformed or empty signatures continue to fall back safely rather than being dereferenced blindly. ## Scope and non-goals - This PR does **not** change GC bridge behavior; that is isolated in #12145. - It does **not** attempt to remove every use of C++ standard-library headers from shared native code. - It does not change CoreCLR behavior where the existing owning containers remain appropriate. - It does not change log categories, log-file naming, update-directory policy, or native diagnostic text beyond how the function name is extracted. ## Expected impact - remove direct owning `basic_string` storage and destructor roots from the NativeAOT logging/path code changed here; - avoid ranges/vector/string materialization in native validation diagnostics; - reduce transient allocations and associated C++ exception/runtime symbol roots; - retain bounded, deterministic storage for process-lifetime NativeAOT paths; - preserve functional behavior while making the remaining libc++ dependency easier to identify and remove. Exact archive/shared-library measurements will be refreshed after CI validates the final split revision. ## Validation The remaining three-file change was included in the earlier combined revision that passed: - `git diff --check`; - Release build of `src/native/native-nativeaot.csproj`; - Release build of `src/native/native-clr.csproj`; - NDK Clang C++23 syntax compilation; - zero-runtime-cost `static_assert` coverage for ordinary functions, scoped methods, templates and GCC substitution suffixes, lambdas, free and scoped operators, malformed signatures, null, and empty input; - focused review of allocation ownership, bounded path copying, NUL termination, CoreCLR/NativeAOT preprocessor scoping, and fallback logging behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
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.