feat(nvtx): add thread_id to RangePush/RangePop and stamp OS thread id in callbacks - #474
Conversation
…d in callbacks Adds thread_id: u32 to the RangePush and RangePop event variants so the analyzer can reconstruct per-thread nested stacks. Stamps the OS thread id via gettid (Linux) in the injection callbacks and proves it end-to-end in a new thread_id capture test. Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughNVTX range push and pop events now carry per-thread identifiers. Injection callbacks capture identifiers through a Linux-specific helper, conversion functions preserve them, and the example adds synchronized multithreaded capture with integration coverage. ChangesNVTX thread ID propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@integrations/nvtx/example/tests/thread_id.rs`:
- Around line 67-72: Expand the test around the single-thread Push/Pop sequence
to coordinate two threads and interleave their operations. Record each thread’s
IDs separately, assert that its matching Push/Pop pairs share one thread_id, and
assert the two threads’ IDs differ; retain the existing consistency checks while
exercising the per-thread reconstruction behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: dcf42e59-a934-4fa3-8f80-bcaaba3459d3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (7)
Cargo.tomlintegrations/nvtx/events/src/lib.rsintegrations/nvtx/example/tests/thread_id.rsintegrations/nvtx/injection/Cargo.tomlintegrations/nvtx/injection/src/callbacks.rsintegrations/nvtx/injection/src/convert.rsintegrations/nvtx/injection/src/init.rs
Replace the single-thread consistency check with a 4-thread test that also asserts cross-thread uniqueness — the property CodeRabbit flagged as missing. Adds run_capture_n_threads to nvtx-example: spawns N threads behind a Barrier so their push/pop events interleave in real time, then asserts N distinct thread_ids across Push events and N across Pop events, and that the two sets are identical (Push and Pop on the same thread share one id). Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
johanpel
left a comment
johanpel
left a comment
There was a problem hiding this comment.
Looks good overall, just two minor things we can improve:
Cache the gettid result (or hash fallback) in a thread_local OnceCell so the syscall is paid once per thread rather than on every push/pop callback. Move libc from [workspace.dependencies] to a direct version pin in nvtx-injection's own Cargo.toml — no other crate uses it. Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
integrations/nvtx/injection/src/init.rs (1)
71-76: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse collision-free IDs for the non-Linux fallback.
Hashing
ThreadIdand truncating the result tou32does not guarantee distinct identifiers for live threads. A collision can merge separate threads’ push/pop stacks during reconstruction;.max(1)only prevents zero. Allocate a process-local nonzero ID (for example, with an atomic counter cached by the existingOnceCell) and add a cross-thread distinctness test.Also applies to: 578-588
🤖 Prompt for 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. In `@integrations/nvtx/injection/src/init.rs` around lines 71 - 76, Replace the hashed ThreadId logic in compute_thread_id with a process-local atomic ID allocator cached through the existing OnceCell, ensuring each live thread receives a distinct nonzero u32 identifier. Reuse the per-thread cached value so repeated calls for one thread remain stable, and add a cross-thread test that verifies the generated IDs are distinct.
🤖 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.
Outside diff comments:
In `@integrations/nvtx/injection/src/init.rs`:
- Around line 71-76: Replace the hashed ThreadId logic in compute_thread_id with
a process-local atomic ID allocator cached through the existing OnceCell,
ensuring each live thread receives a distinct nonzero u32 identifier. Reuse the
per-thread cached value so repeated calls for one thread remain stable, and add
a cross-thread test that verifies the generated IDs are distinct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 36f40791-324d-47f7-a1b8-ca46d14edf64
📒 Files selected for processing (2)
integrations/nvtx/injection/Cargo.tomlintegrations/nvtx/injection/src/init.rs
Remove the non-Linux compute_thread_id hash fallback — the crate already emits a compile_error! in lib.rs for non-Linux-64 targets, so the fallback was unreachable dead code and the hash collision risk (WR-03 / CodeRabbit) was only theoretical. Add an early platform check in build.rs so cargo reports the unsupported target before bindgen or rustc run, with the actual os and pointer_width values in the error message. Linux aarch64 is explicitly supported — gettid and the ELF injection mechanism both work there. Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
|
/merge |
2 participants
Prerequisite for the NVTX tolerant analyzer (#473 ).
Adds
thread_id: u32to theRangePushandRangePopvariants innvtx-eventsso the analyzer can reconstruct per-thread nested stacks. Without a stamped thread id, Push/Pop reconstruction would require a global (incorrect) stack or best-effort heuristics.Changes:
nvtx-events:RangePushandRangePopgain athread_id: u32fieldnvtx-injection:on_range_push/on_range_popcallbacks stamp the OS thread id viagettid(Linux); non-Linux falls back to a hash ofstd::thread::current().id()truncated tou32nvtx-injection:on_domain_range_push_ex/on_domain_range_pop(CORE2 path) updated identicallynvtx-example: newtests/thread_id.rs— captures a two-thread interleaved Push/Pop session and asserts each event carries the expected thread id