fix(trace): emit kernel TIDs and thread-name metadata#15
Conversation
ui.perfetto.dev rendered all spans on a single thread track because std::hash<std::thread::id> produces ~10^18 numbers that exceed JavaScript's safe-integer range (2^53). The traces themselves had distinct tids, but they collapsed to one during JSON parse. Use syscall(SYS_gettid) for the tid (32-bit, matches what perf/ps/top show) and add a set_thread_name helper that emits a ph:'M' metadata event. Label the G4 master and worker threads from geant4_module.cpp so the Perfetto UI shows g4_master and g4_worker_0..N as track names. Verified locally: a 4-worker run now produces 5 distinct kernel TIDs plus 5 thread_name metadata events.
📝 WalkthroughWalkthroughChrome Trace thread identification now uses kernel thread IDs (via ChangesThread identification and naming in trace output
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chrome_trace.hpp`:
- Around line 103-112: The set_thread_name function writes the raw name into
JSON; add a JSON-escaping helper (e.g., escape_json_string or json_escape) and
call it from set_thread_name so that quotes, backslashes, and control characters
are escaped (use \\\", \\\\, \\n, \\r, \\t, and \\uXXXX for others) before
passing to std::fprintf; update references inside set_thread_name (alongside
file_handle(), write_mutex(), first_event(), thread_id()) to use the escaped
string to ensure well-formed output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 506a73c5-7077-431a-bee7-efb9c0969a70
📒 Files selected for processing (2)
src/chrome_trace.hppsrc/geant4_module.cpp
Summary
ui.perfetto.dev was rendering all trace spans on a single thread track
even when the underlying JSON had distinct `tid` values per worker.
Root cause: `chrome_trace.hpp` used `std::hashstd::thread::id` for
the tid, which produces values around 10¹⁸. Perfetto UI parses the
Chrome trace JSON in JavaScript, where the safe-integer limit is
2⁵³ ≈ 9 × 10¹⁵. Our tids were ~1000× past that — JS rounded them
inexactly and Perfetto bucketed the distinct threads into one track.
Fix:
within JavaScript's safe range, and match what `perf`, `ps`, and
`top` show, so cross-referencing with sampling profilers is now
trivial.
event (Chrome Trace "thread_name"), exposed via an
`AEGIR_TRACE_THREAD_NAME` macro.
appear in Perfetto as `g4_master` and `g4_worker_0..N` instead of
bare integers.
Linux-only path (the project is Linux-only). No behavioural change
when `AEGIR_ENABLE_TRACE` is OFF.
Test plan
succeeds.
5 distinct small kernel TIDs (260528, 260529, 260534, 260536,
260541 in the local smoke run) instead of five ~10¹⁸ hash values.
`g4_worker_0`, `g4_worker_1`, `g4_worker_2`, `g4_worker_3`.
five threads with the expected labels (follow-up to existing
investigation; not a CI gate).
Summary by CodeRabbit