[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 |
c54604c to
76813e6
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
Only the thread issuing CALLGRIND_START_INSTRUMENTATION had its shadow call stack seeded from the native one. A thread parked in a syscall at the transition has a non-empty native stack and an empty shadow stack just the same, so its first ret underflows: handleUnderflow pushes the returned-into frame as a fresh top-level context without consulting fn->skip, and an obj-skipped frame becomes a visible root carrying everything the thread runs afterwards. Under pytest-codspeed this put a CPython interpreter frame at the root of every thread-pool worker, holding ~100% of that thread's cost. With CPython 3.14's tail-calling interpreter the leaked frame is a _TAIL_CALL_<OPCODE> handler, named after whichever opcode was mid-execution when the worker parked in queue.get(). Seed all live threads instead, each under its own switch_thread since the call stack, context chain and fn stack live in globals belonging to whichever thread is switched in. A thread other than the running one is unwound from the register state saved when it was descheduled; if that yields a single frame the unwinder never got past the syscall, and seeding one entry with a fabricated entry SP would re-parent the thread's later work under it, so leave it empty. Threads created after the transition are deliberately not covered: they start at their real entry point with a genuinely empty stack. Closes COD-2349 Co-Authored-By: Claude <noreply@anthropic.com>
f33ed05 to
8a969c4
Compare
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