[COD-3196] fix(callgrind): represent os threads under --separate-threads=yes#24
Conversation
Greptile SummaryThis PR fixes Callgrind per-thread dumps for recycled thread slots and mid-run dumps. The main changes are:
Confidence Score: 5/5This looks safe to merge.
|
| Filename | Overview |
|---|---|
| callgrind/main.c | Updates stats zeroing so separate-thread mode resets live threads and clears retired thread state. |
| callgrind/threads.c | Adds stable thread identity, retired-thread tracking, exit handling, and cleanup helpers. |
| callgrind/dump.c | Flushes live and retired thread sections per dump part and emits thread metadata. |
| callgrind/bbcc.c | Uses stable OS thread IDs for BBCC lookup, creation, and clone checks. |
| callgrind/global.h | Extends thread metadata and declares the new thread lifecycle helpers. |
| coregrind/m_threadstate.c | Adds guarded core accessors for thread names and LWP IDs. |
| include/pub_tool_threadstate.h | Exposes thread name and LWP ID accessors to Valgrind tools. |
Reviews (8): Last reviewed commit: "fix(callgrind): represent os threads und..." | Re-trigger Greptile
Merging this PR will not alter performance
Comparing Footnotes
|
3da752a to
2450e56
Compare
2450e56 to
e29c5d7
Compare
e29c5d7 to
8e927b0
Compare
8e927b0 to
5cae7e2
Compare
|
I'll review as well |
bf37129 to
bb4fb76
Compare
Make per-thread dumps behave like OS threads. The valgrind core recycles ThreadId slots and callgrind keyed all per-thread state on that slot, so a new OS thread reusing a slot silently inherited the dead thread's identity and costs. Mid-run client-request dumps also only flushed the calling thread, deferring every other thread's history to the termination dump. - Key thread identity (BBCC lookup, "thread:" header, dump suffix) on a monotonic serial instead of the recycled ThreadId slot. - Retire exiting threads via a pre_thread_ll_exit hook: unwind, snapshot their name, move them to a retired list so their costs stay attributed across the rest of the run. Gated on separate_threads. - Flush every thread, live and retired, at each dump so per-thread deltas land under the part being dumped, in ascending serial order. Threads with a zero delta are skipped; if that skips a whole part (metadata dumps with instrumentation off, termination), force one empty section so the part and its trigger metadata survive. Key the combined-dump header on file state, not out_counter, so it is written even when leading parts are skipped. - Zero every thread on CALLGRIND_ZERO_STATS under separate_threads. - Emit the core's thread name as a "desc: Thread name:" line, picked up by the backend in COD-3197. Refs COD-3196 Co-Authored-By: Claude <noreply@anthropic.com>
bb4fb76 to
febc434
Compare
Unable to generate the flame graphsThe performance report has correctly been generated, but there was an internal error while generating the flame graphs for this run. We're working on fixing the issue. Feel free to contact us on Discord or at support@codspeed.io if the issue persists. |
Make callgrind per-thread dumps (
--separate-threads=yes) behave like OSthreads: a thread spawns, runs, ends, and its costs stay attributed to it —
under the part being dumped, kept separate from other threads that reused its
slot, and tagged with its name.
Why it misbehaved
Two independent causes:
ThreadIdslots on exit.Callgrind keyed all per-thread state — and BBCC identity — on that slot with
no thread-exit hook, so a new OS thread landing on a recycled slot silently
appended its costs to the dead thread's containers.
CALLGRIND_DUMP_STATSdumped onlythe current thread; since dumping is destructive, every other thread's whole
history was deferred to the termination dump, showing up once under the final
part.
What changed
thread_infogets a monotonic serial, independent of the recycledslot; used for the
thread:header and-NNfile suffix.fix for slot reuse — the per-BB LRU cache and hash lookup compared the tid, so
a reused slot otherwise re-merged or tripped the
clone_bbccassertion.pre_thread_ll_exithook unwinds the exiting thread, snapshots its name,and retires it (freed once flushed). Gated on
separate_threads— the=nopath is byte-for-byte untouched.
thread's delta lands under the part being dumped. Zero-delta threads are
skipped uniformly (incl. termination), which cleanly absorbs a thread that
lived entirely inside an instrumentation-off window.
CALLGRIND_ZERO_STATSzeros all threads underseparate_threads.VG_(get_thread_name)tool API surfaces the core's thread name, emittedas a
desc: Thread name:line for the backend (COD-3197) to parse.The retire path is safe on an already-unwound/empty state, so it survives the
STOP/START_INSTRUMENTATIONthe codspeed integrations wrap benchmarks in.Review notes
callgrind/threads.c(retire + retired-list iteration) andcallgrind/dump.c(per-part flush, retired-thread install, zero-delta skip) are the core of the
change;
bbcc.cis the serial re-keying.termination dump also iterates the retired list.
Validation
Two new regression tests:
thread-serial(sequential named workers with a dumpbetween them → two distinct serials, name emitted, costs under the dumped part)
and
thread-instr(instrumentation toggling + an off-window thread → no crash,no spurious section). Full callgrind
make check(25 tests, incl. the existing--separate-threads=yesthreads/threads-use) and cachegrind suite (17,guards the shared core change) pass. Manually re-ran the COD-3188 repros
(sequential, parallel, dump-while-running) — topology as expected.
Draft: opening for early review; will un-draft once the backend side (COD-3197)
is ready to consume the format.
Refs COD-3196