[native] Remove the libc++ dependencies from FastTiming - #12547
Merged
simonrozsival merged 3 commits intoAug 28, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Lite
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
src/native/common/include/runtime-base/timing-internal.hh — Suggestion: the comment hard-codes an observed nesting depth ("currently 3"), which is likely to… |
What changed in this PR
This PR reduces CoreCLR-host libc++/ABI surface area and startup overhead by replacing FastTiming’s thread_local std::stack (deque-backed, non-trivial TLS init/destroy) with a trivially-initialized fixed-capacity array plus a per-thread depth counter.
Changes:
- Replace
thread_local std::stack<TimingEvent*> open_sequenceswiththread_local TimingEvent* open_sequences[MAX_OPEN_SEQUENCES]andthread_local size_t open_sequence_depth. - Introduce
push_sequence_event,get_sequence_event, andpop_sequence_eventhelpers and updatestart_event(and TLS warm-up) to use them. - Add
MAX_OPEN_SEQUENCESconstant and associated documentation describing expected nesting constraints.
| File | Description |
|---|---|
| src/native/common/runtime-base/timing-internal.cc | Switch TLS warm-up to the new fixed-array push/pop helpers. |
| src/native/common/include/runtime-base/timing-internal.hh | Replace deque-backed TLS stack with trivially-initialized fixed array + depth; add helper methods and max-depth constant. |
simonrozsival
force-pushed
the
dev/simonrozsival/timing-open-sequences
branch
2 times, most recently
from
August 28, 2026 07:14
030863a to
ac45516
Compare
simonrozsival
force-pushed
the
dev/simonrozsival/timing-open-sequences
branch
from
August 28, 2026 07:54
ac45516 to
ec975cb
Compare
simonrozsival
force-pushed
the
dev/simonrozsival/timing-open-sequences
branch
from
August 28, 2026 08:47
ec975cb to
da5daa3
Compare
simonrozsival
force-pushed
the
dev/simonrozsival/timing-open-sequences
branch
from
August 28, 2026 08:56
da5daa3 to
460e9a9
Compare
simonrozsival
force-pushed
the
dev/simonrozsival/timing-open-sequences
branch
from
August 28, 2026 09:51
460e9a9 to
74b49e6
Compare
simonrozsival
force-pushed
the
dev/simonrozsival/timing-open-sequences
branch
from
August 28, 2026 10:29
74b49e6 to
df3b56e
Compare
`FastTiming::open_sequences` was a `thread_local std::stack<TimingEvent*>`, which defaults to `std::deque` as its container. `std::deque` has both a non-trivial constructor and a non-trivial destructor, so every translation unit including `timing-internal.hh` emitted a guarded dynamic initializer plus a `__cxa_thread_atexit` registration for the thread-local instance. The stack only ever needs `push`, `top`, `pop` and `empty`, and its depth is bounded by how deeply the instrumented calls nest (currently 3) because every `start_event` is matched by exactly one `end_event` or `store_more_info`. Replace it with a fixed `TimingEvent*` array plus a depth counter, both of which are trivially constructible and destructible and therefore constant initialized. `open_sequences` is `thread_local`, so it is private to each thread and needs no locking - that remains true here, as no state is shared between threads. The depth counter is incremented even when the array is full, so a push past the bound only loses that one entry instead of misaligning the pairing of the events below it. Once the depth drops back within bounds the remaining entries are still correct. Removes all 4 `__cxa_thread_atexit` references and one `__libcpp_verbose_abort`, taking the CoreCLR host's libc++ references from 64 to 59. As a side effect, pushing a timing event no longer allocates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
The fixed array capped the nesting depth of timing events, which is not a
limit the timing code should impose - any number of events may be open on a
thread at once. Replace it with a naive singly linked list used as a stack,
with one malloc'd node per open sequence:
struct OpenSequence
{
TimingEvent *event;
OpenSequence *next;
};
static inline thread_local OpenSequence *open_sequences = nullptr;
The head pointer is still a trivially destructible thread-local, so this keeps
the property that motivated the change: no guarded dynamic initializer and no
`__cxa_thread_atexit` registration.
Nodes are freed as they are popped rather than being recycled, so a thread
that balances its `start_event` and `end_event` calls leaves nothing behind
when it exits. That matters here because, unlike the process-wide timing
sequence pool, this list is per thread and threads come and go.
Allocation failure aborts, matching how the timing sequence chunks behave.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
`FastTiming` kept two heap-allocated `std::string`s that the earlier pass over the timing code missed: the per-event `TimingEvent::more_info` and the output file name parsed out of the `debug.mono.timing` property. `more_info` becomes a plain NUL-terminated `char*`. It was always built from one or two `std::string_view`s whose total length is known up front, so a single `malloc` and one or two `memcpy`s replace the string entirely. When the allocation fails we simply drop the extra information instead of aborting - timing is a diagnostic facility and must not take the application down with it. The output file name comes from a system property, whose value is limited to `PROP_VALUE_MAX` (92) bytes, so it now lives in a fixed 128 byte buffer inside `FastTiming` rather than in a `std::unique_ptr<std::string>`. Keeping it inline also means the global `internal_timing` instance stays constant-initialized and needs no guard variable. Names that do not fit are rejected with a warning and the default is used. Together with the previous commit this removes the last `operator new` and `operator delete` references from `timing-internal.cc.o` and, as a side effect, all of them from `typemap.cc.o`, which had been inheriting them from the inlined `new TimingEventChunk` in `FastTiming::get_event`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0a35a0db-502d-48c0-8468-e73b5dd0ab2e
simonrozsival
force-pushed
the
dev/simonrozsival/timing-open-sequences
branch
from
August 28, 2026 12:06
df3b56e to
c6c4047
Compare
Member
Author
|
Consolidated into #12545 to reduce the depth of the #12546 stack. No code changed: the commits from this PR are now part of #12545 unmodified, and the resulting tree is byte-identical. This PR sat directly on top of #12545 and touched the same files, so reviewing them together is easier than reviewing the same file across two intermediate states. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Part of #12533. Stacked on top of #12545.
The problem
FastTiming::open_sequencestracks the timing events which have been started but not yet ended on each thread:std::stackdefaults tostd::dequeas its underlying container, andstd::dequehas both a non-trivial constructor and a non-trivial destructor. For athread_local, that means every translation unit which includestiming-internal.hhemits a guarded dynamic initializer for it, plus a__cxa_thread_atexitregistration so the deque is destroyed when the thread exits.timing-internal.hhis included by both the CoreCLR and the MonoVM hosts.The change
The stack is only ever used through
push,top,popandempty, so a naive singly linked list is enough — no need for libc++ here:The head pointer is a plain pointer, so it is trivially destructible and constant initialized: no guard variable, and nothing to register with
__cxa_thread_atexit. The list itself is unbounded — any number of events may be open at once, exactly as before.Locking:
open_sequencesisthread_local, so the list is private to its thread and needs no lock. This change keeps it that way — no state moves into shared storage — so there is still nothing to synchronize.Lifetime: nodes are freed as they are popped rather than being recycled. That matters here because, unlike the process-wide timing sequence pool in #12545, this list is per thread and threads come and go — recycling nodes would mean every thread that ever recorded a timing event left its nodes behind. A thread that balances its
start_eventandend_eventcalls now leaves nothing allocated when it exits. Allocation failure aborts, matching how the timing sequence chunks behave.Results
Undefined libc++ references across the three archives the CoreCLR host links,
libnet-android.release-static-release.a,libruntime-base-release.aandlibruntime-base-common-release.a:__cxa_thread_atexitstd::__ndk1::__libcpp_verbose_abort(char const*, ...)This removes the
__cxa_thread_atexitcategory entirely.Verification
format_managed_type_namewarning).llvm-nm --undefined-onlyover all three archives, before and after, on the same build tree.push/top/pop/emptysemantics were checked against a standalone harness with instrumentedmalloc/free, covering the empty, balanced-LIFO, interleaved, over-pop and 100 000-deep cases, asserting strict LIFO order and that every node is freed. 20/20 pass.src/native/mono/is untouched.Also: the two
std::strings the earlier timing pass missed#12513 removed the local strings from the timing code, but two heap-allocated ones survived in
FastTimingand were only spotted later while attributing the remaininglibc++references. They live in the same two files as the change above, so they are fixed here.TimingEvent::more_infostd::string *more_info = nullptr;It is always built from one or two
std::string_views whose combined length is known up front, so it becomes a plain NUL-terminatedchar*produced by a singlemallocplus one or twomemcpys. If the allocation fails we drop the extra information for that one event rather than aborting — timing is a diagnostic facility and must not take the application down with it.FastTiming::output_file_namestd::unique_ptr<std::string> output_file_name{};The name is parsed out of the
debug.mono.timingsystem property, whose entire value is capped atPROP_VALUE_MAX(92) bytes, so a fixed 128 byte buffer insideFastTimingis always large enough. Keeping it inline also keeps the globalinternal_timinginstance constant-initialized, so it needs no guard variable. A name that does not fit is rejected with a warning and the default is used.<memory>and<string>are no longer needed bytiming-internal.hhat all.Result
Together with the
calloccommit added to #12545, this clears the lastoperator new/operator deletereferences fromtiming-internal.cc.o(2 → 0) and, as a side effect, fromtypemap.cc.o(2 → 0) —typemap.cchad been inheriting them purely from the inlinednew TimingEventChunkinFastTiming::get_event.At the tip of the stack the CoreCLR total goes from 26 to 22, and only two objects still reference
libc++:host.cc.o(11) andassembly-store.cc.o(11).