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 fixes Callgrind per-thread dumps for recycled thread slots and mid-run dumps. The main changes are:

  • Tracks a stable OS thread ID on each Callgrind thread record.
  • Re-keys BBCC ownership checks on the stable thread ID.
  • Retires exited thread state so it can be flushed after exit.
  • Flushes live and retired threads for each dump part in separate-thread mode.
  • Drops retired thread state when stats are zeroed.
  • Adds tool APIs for reading live thread names and LWP IDs.

Confidence Score: 5/5

This looks safe to merge.

  • The retired-thread zero boundary is now handled by resetting live threads and dropping exited thread state.
  • No blocking issues were found in the changed code.

Important Files Changed

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

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 not alter performance

✅ 84 untouched benchmarks
⏩ 60 skipped benchmarks1


Comparing cod-3196-fix-valgrind-separate-threadsyes-behavior-to-represent-os (febc434) with master (448b7d0)

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 2 times, most recently from bf37129 to bb4fb76 Compare July 22, 2026 21:56
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 bb4fb76 to febc434 Compare July 27, 2026 12:27
@codspeed-hq

codspeed-hq Bot commented Jul 27, 2026

Copy link
Copy Markdown

Unable to generate the flame graphs

The 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.

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