Skip to content

[COD-3196] fix(callgrind): represent os threads under --separate-threads=yes#24

Open
GuillaumeLagrange wants to merge 1 commit into
masterfrom
cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os
Open

[COD-3196] fix(callgrind): represent os threads under --separate-threads=yes#24
GuillaumeLagrange wants to merge 1 commit into
masterfrom
cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os

Conversation

@GuillaumeLagrange

Copy link
Copy Markdown

Make callgrind per-thread dumps (--separate-threads=yes) behave like OS
threads: 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:

  1. Slot reuse. The valgrind core recycles ThreadId slots 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.
  2. Mid-run dumps only flushed the caller. CALLGRIND_DUMP_STATS dumped only
    the 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

  • Each thread_info gets a monotonic serial, independent of the recycled
    slot; used for the thread: header and -NN file suffix.
  • BBCC identity re-keyed on the serial (was the raw tid). This is the real
    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_bbcc assertion.
  • A pre_thread_ll_exit hook unwinds the exiting thread, snapshots its name,
    and retires it (freed once flushed). Gated on separate_threads — the =no
    path is byte-for-byte untouched.
  • Every thread is flushed at each part dump (live + retired), so each
    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_STATS zeros all threads under separate_threads.
  • New VG_(get_thread_name) tool API surfaces the core's thread name, emitted
    as 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_INSTRUMENTATION the codspeed integrations wrap benchmarks in.

Review notes

  • callgrind/threads.c (retire + retired-list iteration) and callgrind/dump.c
    (per-part flush, retired-thread install, zero-delta skip) are the core of the
    change; bbcc.c is the serial re-keying.
  • The main thread is itself retired on its own exit before finalisation, so the
    termination dump also iterates the retired list.

Validation

Two new regression tests: thread-serial (sequential named workers with a dump
between 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=yes threads/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

@greptile-apps

greptile-apps Bot commented Jul 20, 2026

Copy link
Copy Markdown

Greptile Summary

This PR updates Callgrind per-thread dumps to keep exited OS threads separate. The main changes are:

  • Thread dump identity now uses a stable kernel thread id instead of the recycled Valgrind thread slot.
  • Exited threads are retired, named, flushed with later part dumps, and then freed.
  • Per-part dumps now flush live and retired threads under --separate-threads=yes.
  • CALLGRIND_ZERO_STATS now resets live per-thread state and drops retired state.
  • Core threadstate helpers now expose live thread names and LWP ids to tools.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
callgrind/main.c Updates zeroing so separate-thread mode resets live threads and drops retired thread state.
callgrind/threads.c Adds stable thread identity, exit retirement, retired-thread iteration, and retired-state cleanup.
callgrind/dump.c Flushes live and retired threads per dump part, emits thread names, and skips empty thread deltas.
callgrind/bbcc.c Uses stable thread identity for BBCC lookup, creation, and clone checks.
callgrind/global.h Adds thread metadata and declarations for the new retirement and identity helpers.
coregrind/m_threadstate.c Adds core helpers for reading a thread name and LWP id.
include/pub_tool_threadstate.h Declares the new public threadstate helper APIs for tools.

Reviews (7): Last reviewed commit: "fix(callgrind): represent os threads und..." | Re-trigger Greptile

Comment thread callgrind/main.c
Comment thread callgrind/tests/thread-serial.c Outdated
Comment thread callgrind/tests/thread-instr.c Outdated
@codspeed-hq

codspeed-hq Bot commented Jul 20, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by ×13

⚡ 1 improved benchmark
✅ 83 untouched benchmarks
⏩ 60 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
WallTime test_valgrind[valgrind-3.25.1, python3 testdata/test.py, inline] 61.2 s 4.7 s ×13

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.


Comparing cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os (bb4fb76) with master (76a6a64)

Open in CodSpeed

Footnotes

  1. 60 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os branch from 3da752a to 2450e56 Compare July 21, 2026 09:23
Comment thread callgrind/main.c
Comment thread callgrind/tests/thread-serial.c Outdated
Comment thread callgrind/tests/thread-instr.c Outdated
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os branch from 2450e56 to e29c5d7 Compare July 21, 2026 15:07
Comment thread callgrind/main.c
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os branch from e29c5d7 to 8e927b0 Compare July 21, 2026 15:11
Comment thread callgrind/main.c
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os branch from 8e927b0 to 5cae7e2 Compare July 21, 2026 15:49
@GuillaumeLagrange
GuillaumeLagrange marked this pull request as ready for review July 22, 2026 07:48
@art049

art049 commented Jul 22, 2026

Copy link
Copy Markdown
Member

I'll review as well

@art049
art049 self-requested a review July 22, 2026 08:13

@not-matthias not-matthias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread callgrind/global.h Outdated
Comment thread callgrind/threads.c Outdated
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os branch from 5cae7e2 to bf37129 Compare July 22, 2026 21:23
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>
@GuillaumeLagrange
GuillaumeLagrange force-pushed the cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os branch from bf37129 to bb4fb76 Compare July 22, 2026 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants