[NativeAOT] Reduce owning C++ standard library state#12142
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 187b207a-083b-461e-9071-e9aab61c9d07
There was a problem hiding this comment.
Pull request overview
Reduces owning C++ standard-library state reachable from the Android NativeAOT host to help shrink binaries and remove libc++-related symbol roots (part of #12139).
Changes:
- Replaces GC-bridge temporary peer storage from
std::unordered_maptotsl::robin_map, and tightens lifetime cleanup. - Removes/avoids owning
std::stringstorage in logging paths and source-location function-name parsing (moves to boundedstd::string_viewparsing). - Introduces NativeAOT-specific bounded storage for Android override/native-library paths (while keeping CoreCLR behavior).
Show a summary per file
| File | Description |
|---|---|
| src/native/common/include/shared/cpp-util.hh | Reworks function-name extraction to bounded std::string_view parsing and updates formatted error messages to avoid owning strings. |
| src/native/CMakeLists.txt | Makes robin-map include path available to CLR/NativeAOT targets and unconditionally defines ROBIN_MAP_DIR. |
| src/native/clr/runtime-base/logger.cc | Switches gref/lref path storage away from owning std::string to explicit malloc/free management and uses bounded path building. |
| src/native/clr/include/runtime-base/android-system.hh | Adds NativeAOT-specific bounded path storage/return types for override/native-library directories. |
| src/native/clr/include/host/bridge-processing-shared.hh | Swaps temporary peer map to tsl::robin_map and adds guarded include/diagnostic handling. |
| src/native/clr/host/bridge-processing.cc | Updates temporary-peer insertion/iteration for robin-map and ensures local refs are deleted before clearing the map. |
Copilot's findings
- Files reviewed: 6/6 changed files
- Comments generated: 1
Use the negative-index TemporaryPeerMap approach from #11311 so CoreCLR and NativeAOT no longer need robin-map for GC bridge processing. 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
CI failure analysisThe public The sole failure was The final link reports duplicate symbols such as
This is the same pre-existing collision independently reproduced by #12140 build #1513800. It is unrelated to this PR's container/string cleanup and provides further evidence for #12139. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70f63eb7-6599-414c-a947-d860705aa0fa
Preserve the NativeAOT fixed-buffer state while keeping the new CoreCLR application code-cache directory state behind the non-NativeAOT path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f8e6959-6647-4d07-ab74-492a1e60d3ec
|
/reivew |
|
/review |
|
✅ Android PR Reviewer completed successfully! |
jonathanpeppers
left a comment
There was a problem hiding this comment.
Review summary
✅ LGTM with two suggestions
The change cleanly removes owning std::string/std::vector/ranges state from three NativeAOT-reachable code paths, replacing them with bounded process-lifetime storage and non-owning std::string_view. I independently verified the risky bits:
- logger.cc —
get_log_file_namenow returnsstd::string_view { value(), segment.length() - offset }.string_segment::at()returns_start + offsetandlength()is the token length, so the computed length is correct; empty paths correctly clear the config viaset_log_file. The%.*susage bounds the non-NUL-terminated view correctly. ✅ - cpp-util.hh — the hand-rolled
get_function_nameparser isconstexpr/noexcept, allsubstrcalls are bounds-guarded, and it is well covered bystatic_asserts (ordinary/scoped/template/lambda/operator/malformed/null/empty). ✅ - android-system.hh — the NativeAOT
determine_primary_override_dircopies into a fixed buffer with anabort_unlesslength check beforememcpy(..., len+1); preprocessor nesting is balanced. ✅ CoreCLR paths (clr/host/host.cc,clr/runtime-base/android-system.cc) are unaffected since they are not in the NativeAOT source set.
CI: Azure DevOps build #1515222 succeeded and all GitHub checks are green.
Nice work overall: the static_assert coverage on get_function_name and the bounded-copy + abort_unless discipline are exactly right.
Remove the unused NativeAOT native libraries path storage and release temporary reference logging path buffers after initialization. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1378b7cc-f45b-47c6-bb09-ac55670c55a3
## 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 #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`.
## 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:
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:
std::stringinstances;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.ccstd::stringinstances forgref_fileandlref_filewith nullable C-string pointers;set_log_file()to perform overflow-checked allocation, copy the selected path, NUL-terminate it, and release the previous value;gref=/lref=values asstd::string_viewinstead of returning a pointer into the parser without a length;FILE*;<override>/grefs.txtin boundeddynamic_local_string<SENSIBLE_PATH_MAX>storage instead of an owningstd::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.hhUnder
XA_HOST_NATIVEAOT:primary_override_dirandnative_libraries_dirin fixed process-lifetime buffers sized byConstants::SENSIBLE_PATH_MAX;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.hhget_function_name();std::string_view;%.*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
Expected impact
basic_stringstorage and destructor roots from the NativeAOT logging/path code changed here;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;src/native/native-nativeaot.csproj;src/native/native-clr.csproj;static_assertcoverage for ordinary functions, scoped methods, templates and GCC substitution suffixes, lambdas, free and scoped operators, malformed signatures, null, and empty input;CI is validating head
4412ea395.