feat(nvtx): tolerant NVTX model and analyzer - #473
Conversation
8324a98 to
b4f3b93
Compare
…d in callbacks (#474) Prerequisite for the NVTX tolerant analyzer (#473 ). Adds `thread_id: u32` to the `RangePush` and `RangePop` variants in `nvtx-events` so 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`: `RangePush` and `RangePop` gain a `thread_id: u32` field - `nvtx-injection`: `on_range_push` / `on_range_pop` callbacks stamp the OS thread id via `gettid` (Linux); non-Linux falls back to a hash of `std::thread::current().id()` truncated to `u32` - `nvtx-injection`: `on_domain_range_push_ex` / `on_domain_range_pop` (CORE2 path) updated identically - `nvtx-example`: new `tests/thread_id.rs` — captures a two-thread interleaved Push/Pop session and asserts each event carries the expected thread id Authors: - Pradeep Garigipati (https://github.com/9prady9) Approvers: - Johan Peltenburg (https://github.com/johanpel) URL: #474
ff92c45 to
f87f367
Compare
|
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)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdded the ChangesNVTX analyzer
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 4
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
integrations/nvtx/analyzer/tests/roundtrip.rs-54-57 (1)
54-57: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Arc::try_unwrapassumes the capture layer drops the callback.
sinkholds a secondArcclone inside its closure.Arc::try_unwrapsucceeds only ifrun_capturedrops that callback before it returns. The comment on lines 50-51 states that injection is process-global and one-shot, which suggests the registry can retain the callback for the life of the process. If it does, this line panics with "sink outlived the capture" and the test fails for a reason unrelated to reconstruction.Read the events through the
Arcinstead. This removes the ownership assumption.🛡️ Proposed fix
- let events = Arc::try_unwrap(collected) - .expect("sink outlived the capture") - .into_inner() - .expect("collector poisoned"); + let events: Vec<Event<NvtxEventEntity>> = std::mem::take( + &mut *collected.lock().expect("collector poisoned"), + );🤖 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/analyzer/tests/roundtrip.rs` around lines 54 - 57, Update the event-reading logic after run_capture to avoid Arc::try_unwrap on collected, since the capture callback may retain another Arc clone; read the collected events through the shared Arc while preserving the existing poisoned-lock handling and reconstruction flow.integrations/nvtx/analyzer/src/ranges.rs-85-96 (1)
85-96: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winOverwriting an open entry silently drops a span in both reconstructions. Both modules register an open interval with
HashMap::insertand treat the returnedSome(previous)as a log line only. The previous open entry is discarded, so an observed start or create produces no span at all. This contradicts the stated contract that an open which is never closed is closed at trace end and flagged synthetic. Return the displaced entry as a synthetically closed span instead of dropping it.
integrations/nvtx/analyzer/src/ranges.rs#L85-L96: changeStartEndRanges::startto returnOption<NvtxSpan>, close the displacedOpenStartRangeat the restart timestamp withsynthetic_end = true, and push the returned span intoslotsat theRangeStartarm insrc/model.rs.integrations/nvtx/analyzer/src/resource.rs#L126-L146: changeResources::createto returnOption<NvtxSpan>, close the displacedOpenResourceat the recreate timestamp withsynthetic_end = true, and push the returned span intoslotsat theResourceCreatearm insrc/model.rs.Add a test for each path. New Rust components must include accompanying tests.
🤖 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/analyzer/src/ranges.rs` around lines 85 - 96, The open-entry overwrite handling must preserve displaced spans in both modules. In integrations/nvtx/analyzer/src/ranges.rs:85-96, update StartEndRanges::start to return Option<NvtxSpan>, synthetically close any displaced OpenStartRange at the restart timestamp, and append the returned span in the RangeStart arm of src/model.rs; add a test. In integrations/nvtx/analyzer/src/resource.rs:126-146, apply the same behavior to Resources::create and append its result in the ResourceCreate arm of src/model.rs; add a test.Source: Coding guidelines
🧹 Nitpick comments (4)
integrations/nvtx/analyzer/src/resource.rs (1)
148-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLower the log level for the orphan destroy.
The doc at Lines 150-152 states that an orphan destroy is the normal case for a resource created before capture attached. The code logs it at
warn!. A routine condition atwarnlevel produces noise and reduces the signal of the genuine anomalies logged elsewhere in this crate. Usedebug!here and keepwarn!for the unexpected paths.♻️ Proposed change
-use tracing::warn; +use tracing::{debug, warn};let Some(open) = self.open.remove(&handle) else { - warn!( - "orphan nvtx resource destroy for handle 0x{handle:X} with no open create; skipping" - ); + debug!( + "orphan nvtx resource destroy for handle 0x{handle:X} with no open create; \ + the create predates capture attach" + ); return None; };🤖 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/analyzer/src/resource.rs` around lines 148 - 159, In Resource::destroy, change the orphan-destroy log from warn! to debug! while preserving its message and early None return; leave warn! logging for genuinely unexpected paths unchanged.integrations/nvtx/analyzer/src/stats.rs (1)
109-121: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid the per-span
StatsKeyallocation on the hot fold.Each iteration clones
span.nameeven when the group already exists. For large traces this allocates oneStringper span. Look up first, then build the key only on a miss.♻️ Proposed refactor
for span in spans .iter() .filter(|span| matches!(span.kind, SpanKind::PushPop | SpanKind::StartEnd)) { - grouped - .entry(StatsKey { - name: span.name.clone(), - domain: span.domain, - category: span.category, - }) - .or_default() - .accumulate(span); + let key = StatsKey { + name: span.name.clone(), + domain: span.domain, + category: span.category, + }; + match grouped.get_mut(&key) { + Some(stats) => stats.accumulate(span), + None => grouped.entry(key).or_default().accumulate(span), + } }The clone still happens on a miss only if you build the key lazily; a borrowed lookup key type (for example a
(&str, u64, Option<u32>)index map) removes it entirely.🤖 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/analyzer/src/stats.rs` around lines 109 - 121, Update the grouping fold around grouped and StatsKey to avoid cloning span.name on every iteration: first perform a borrowed lookup using span.name, domain, and category, accumulating into the existing group when found; only construct an owned StatsKey, including the name clone, when inserting a missing group.integrations/nvtx/analyzer/src/model.rs (2)
165-170: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the "no empty slot" invariant enforced, not documented.
flatten()drops any remainingNoneand shifts every later index.SpanIdvalues were handed out against the pre-flatten indices, so a single unfilled slot silently repoints everyparentin the model. The invariant holds today, because a push is filled by its pop or byclose_at_trace_end. It is not enforced by any type or check. Add a debug assertion so a future change fails loudly.🛡️ Proposed guard
+ debug_assert!( + slots.iter().all(Option::is_some), + "an unfilled span slot would shift every SpanId assigned after it" + ); let spans: Vec<NvtxSpan> = slots.into_iter().flatten().collect();Also applies to: 258-261
🤖 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/analyzer/src/model.rs` around lines 165 - 170, Add a debug assertion in flatten() before removing or compacting slots to verify every entry in the slots Vec<Option<NvtxSpan>> is Some, preserving SpanId indices and failing loudly if an unfilled slot is introduced; apply the same guard at the corresponding trace-end drain path around the other slots declaration.
240-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the catch-all arm with explicit variants.
Match
DomainCreate,DomainDestroy,RegisterString,NameCategory, andNameThreadexplicitly. This preserves the pass 1 behavior and makes newNvtxEventvariants fail compilation instead of being discarded.🤖 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/analyzer/src/model.rs` around lines 240 - 243, Update the match containing the catch-all `_ => {}` arm to explicitly handle NvtxEvent variants DomainCreate, DomainDestroy, RegisterString, NameCategory, and NameThread with the existing no-op behavior. Remove the wildcard arm so any newly added NvtxEvent variant causes a compile-time exhaustiveness error.
🤖 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/analyzer/src/model.rs`:
- Around line 145-147: Update the NvtxModelBuilder::build method to return
NvtxModel directly instead of NvtxModelResult<NvtxModel>, since its
implementation only constructs and returns a successful value. Change the
corresponding Ok(...) return and remove unnecessary error handling from its
callers and tests, including the .expect("build") usage.
In `@integrations/nvtx/analyzer/tests/reconstruction.rs`:
- Around line 12-23: The span lookup helper is duplicated across both test
targets; move it into shared fixtures and import it at each use site. In
integrations/nvtx/analyzer/tests/fixtures.rs, add the helper as pub fn span; in
integrations/nvtx/analyzer/tests/reconstruction.rs lines 12-23 and
integrations/nvtx/analyzer/tests/pushpop.rs lines 18-29, remove the local
definitions and add span to each fixtures import. Remove NvtxModel and NvtxSpan
imports where no longer needed, retaining them in pushpop.rs only if span_id
still uses them.
In `@integrations/nvtx/analyzer/tests/resource.rs`:
- Around line 17-20: Update the shared fixtures module usage in this integration
test so unused public helpers do not trigger dead_code warnings under -D
warnings. In the test setup around `mod fixtures` and the
`fixtures::{range_push, resource_create, resource_destroy}` imports, limit
compilation or exposure to the helpers used by this binary while preserving the
separate helpers required by `tests/stats.rs`.
In `@integrations/nvtx/analyzer/tests/stats.rs`:
- Around line 14-18: Update the shared `fixtures` module declaration used by the
test binaries to allow unused fixture helpers under `-D warnings`, so helpers
imported only by other binaries do not trigger `dead_code`; preserve the
existing imports in `stats.rs` and `tests/resource.rs`.
---
Other comments:
In `@integrations/nvtx/analyzer/src/ranges.rs`:
- Around line 85-96: The open-entry overwrite handling must preserve displaced
spans in both modules. In integrations/nvtx/analyzer/src/ranges.rs:85-96, update
StartEndRanges::start to return Option<NvtxSpan>, synthetically close any
displaced OpenStartRange at the restart timestamp, and append the returned span
in the RangeStart arm of src/model.rs; add a test. In
integrations/nvtx/analyzer/src/resource.rs:126-146, apply the same behavior to
Resources::create and append its result in the ResourceCreate arm of
src/model.rs; add a test.
In `@integrations/nvtx/analyzer/tests/roundtrip.rs`:
- Around line 54-57: Update the event-reading logic after run_capture to avoid
Arc::try_unwrap on collected, since the capture callback may retain another Arc
clone; read the collected events through the shared Arc while preserving the
existing poisoned-lock handling and reconstruction flow.
---
Nitpick comments:
In `@integrations/nvtx/analyzer/src/model.rs`:
- Around line 165-170: Add a debug assertion in flatten() before removing or
compacting slots to verify every entry in the slots Vec<Option<NvtxSpan>> is
Some, preserving SpanId indices and failing loudly if an unfilled slot is
introduced; apply the same guard at the corresponding trace-end drain path
around the other slots declaration.
- Around line 240-243: Update the match containing the catch-all `_ => {}` arm
to explicitly handle NvtxEvent variants DomainCreate, DomainDestroy,
RegisterString, NameCategory, and NameThread with the existing no-op behavior.
Remove the wildcard arm so any newly added NvtxEvent variant causes a
compile-time exhaustiveness error.
In `@integrations/nvtx/analyzer/src/resource.rs`:
- Around line 148-159: In Resource::destroy, change the orphan-destroy log from
warn! to debug! while preserving its message and early None return; leave warn!
logging for genuinely unexpected paths unchanged.
In `@integrations/nvtx/analyzer/src/stats.rs`:
- Around line 109-121: Update the grouping fold around grouped and StatsKey to
avoid cloning span.name on every iteration: first perform a borrowed lookup
using span.name, domain, and category, accumulating into the existing group when
found; only construct an owned StatsKey, including the name clone, when
inserting a missing group.
🪄 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: 05b9dd7b-a823-4337-8ee8-1a8dc4ee43a0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (17)
Cargo.tomlintegrations/nvtx/analyzer/Cargo.tomlintegrations/nvtx/analyzer/src/error.rsintegrations/nvtx/analyzer/src/lib.rsintegrations/nvtx/analyzer/src/model.rsintegrations/nvtx/analyzer/src/ranges.rsintegrations/nvtx/analyzer/src/resource.rsintegrations/nvtx/analyzer/src/span.rsintegrations/nvtx/analyzer/src/stats.rsintegrations/nvtx/analyzer/src/tables.rsintegrations/nvtx/analyzer/tests/fixtures.rsintegrations/nvtx/analyzer/tests/pushpop.rsintegrations/nvtx/analyzer/tests/reconstruction.rsintegrations/nvtx/analyzer/tests/resolution.rsintegrations/nvtx/analyzer/tests/resource.rsintegrations/nvtx/analyzer/tests/roundtrip.rsintegrations/nvtx/analyzer/tests/stats.rs
f87f367 to
26e3c91
Compare
9prady9
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
integrations/nvtx/analyzer/tests/fixtures.rs (1)
1-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove shared fixtures out of
tests/to avoid a spurious test binary.Cargo auto-registers every
.rsfile placed directly undertests/as its own integration-test binary.fixtures.rshas no#[test]functions, socargo testcompiles and runs an extra binary that reports zero tests, on top of the separate compilation triggered by eachmod fixtures;declaration in reconstruction.rs, resource.rs, and stats.rs.Move the file to a subdirectory, for example
tests/common/mod.rs, and reference it from each consumer with#[path = "common/mod.rs"] mod fixtures;(ormod common;). Cargo only auto-discovers.rsfiles directly undertests/, not files nested in subdirectories, so this removes the extra binary target.Please confirm current Cargo test-target discovery behavior for files nested under
tests/<subdir>/, since this affects whether the proposed restructuring avoids the extra binary target.🤖 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/analyzer/tests/fixtures.rs` around lines 1 - 18, Move the shared fixtures module from the top-level tests directory into a nested common subdirectory so Cargo does not register it as a standalone integration-test target; files nested under tests subdirectories are not auto-discovered as test binaries. Update the fixture imports in reconstruction, resource, and stats to reference the relocated module via an explicit path or shared common module, and confirm cargo test no longer reports the zero-test fixtures binary.integrations/nvtx/analyzer/src/ranges.rs (2)
131-144: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnbounded
warn!volume at trace end inintegrations/nvtx/analyzer/src/ranges.rsandintegrations/nvtx/analyzer/src/resource.rs. Both trace-end cleanup paths log onewarn!per still-open entity, so log output scales with the number of leaked ranges and resources. Replace the per-item warning with one aggregated warning and keep the per-item detail atdebug!.
integrations/nvtx/analyzer/src/ranges.rs#L131-L144: log a singlewarn!with the leaked range count inStartEndRanges::close_at_trace_end, and move the per-range message todebug!.integrations/nvtx/analyzer/src/resource.rs#L144-L158: log a singlewarn!with the leaked resource count inResources::close_at_trace_end, and move the per-handle message todebug!.🤖 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/analyzer/src/ranges.rs` around lines 131 - 144, In integrations/nvtx/analyzer/src/ranges.rs:131-144, update StartEndRanges::close_at_trace_end to emit one warn! containing the leaked range count, and change each per-range warning to debug!. Apply the same change in integrations/nvtx/analyzer/src/resource.rs:144-158 within Resources::close_at_trace_end: aggregate the leaked resource count in a single warn! and move per-handle details to debug!.
119-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLower the orphan
RangeEndlog level todebug!.An orphan
RangeEndis routine, not anomalous. If capture attaches after a range starts, the end arrives with no recorded start.Resources::destroyinintegrations/nvtx/analyzer/src/resource.rs(Lines 128-136) already treats the identical situation as routine and usesdebug!, with the reason stated in the comment. Use the same level here so genuine anomalies stay visible.♻️ Proposed change
- pub(crate) fn end(&mut self, range_id: u64, end: TimeUnixNanoSec) -> Option<NvtxSpan> { - let Some(open) = self.open.remove(&range_id) else { - warn!("orphan nvtx range end for id 0x{range_id:X} with no open start; skipping"); - return None; - }; + pub(crate) fn end(&mut self, range_id: u64, end: TimeUnixNanoSec) -> Option<NvtxSpan> { + let Some(open) = self.open.remove(&range_id) else { + // `debug!`, not `warn!`: a range started before capture attached + // always ends without a recorded start. + debug!("orphan nvtx range end for id 0x{range_id:X} with no open start; skipping"); + return None; + };Add
debugto thetracingimport at Line 20.🤖 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/analyzer/src/ranges.rs` around lines 119 - 125, In the end method, lower the orphan NVTX range-end message from warn! to debug!, and add the corresponding debug import from tracing while preserving the existing skip behavior.
🤖 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/analyzer/tests/roundtrip.rs`:
- Around line 1-128: Before merging, obtain and record Quent maintainer approval
for Issue `#372`, add a valid Signed-off-by line to commit
26e3c91f603a1fed12befa91e39ad2a5b29d412f, and wait for the Rust, quent-open
compatibility, and UI E2E checks to complete successfully.
---
Nitpick comments:
In `@integrations/nvtx/analyzer/src/ranges.rs`:
- Around line 131-144: In integrations/nvtx/analyzer/src/ranges.rs:131-144,
update StartEndRanges::close_at_trace_end to emit one warn! containing the
leaked range count, and change each per-range warning to debug!. Apply the same
change in integrations/nvtx/analyzer/src/resource.rs:144-158 within
Resources::close_at_trace_end: aggregate the leaked resource count in a single
warn! and move per-handle details to debug!.
- Around line 119-125: In the end method, lower the orphan NVTX range-end
message from warn! to debug!, and add the corresponding debug import from
tracing while preserving the existing skip behavior.
In `@integrations/nvtx/analyzer/tests/fixtures.rs`:
- Around line 1-18: Move the shared fixtures module from the top-level tests
directory into a nested common subdirectory so Cargo does not register it as a
standalone integration-test target; files nested under tests subdirectories are
not auto-discovered as test binaries. Update the fixture imports in
reconstruction, resource, and stats to reference the relocated module via an
explicit path or shared common module, and confirm cargo test no longer reports
the zero-test fixtures binary.
🪄 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: 2044582d-774d-4ec6-bcbb-3e959b8bd6d2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (16)
Cargo.tomlintegrations/nvtx/analyzer/Cargo.tomlintegrations/nvtx/analyzer/src/lib.rsintegrations/nvtx/analyzer/src/model.rsintegrations/nvtx/analyzer/src/ranges.rsintegrations/nvtx/analyzer/src/resource.rsintegrations/nvtx/analyzer/src/span.rsintegrations/nvtx/analyzer/src/stats.rsintegrations/nvtx/analyzer/src/tables.rsintegrations/nvtx/analyzer/tests/fixtures.rsintegrations/nvtx/analyzer/tests/pushpop.rsintegrations/nvtx/analyzer/tests/reconstruction.rsintegrations/nvtx/analyzer/tests/resolution.rsintegrations/nvtx/analyzer/tests/resource.rsintegrations/nvtx/analyzer/tests/roundtrip.rsintegrations/nvtx/analyzer/tests/stats.rs
26e3c91 to
644ea7a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
integrations/nvtx/analyzer/src/tables.rs (1)
80-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the order-independence claim.
The doc states that every accumulation is "an idempotent insert or a
min/max".domain_names,registered_strings,category_names, andthread_namesuse last-writer-wins inserts. If a stream registers the same key twice with different values, the result depends on scan order. The result stays deterministic only becausebuildreceives a timestamp-ordered slice fromNvtxModelBuilder::build. State that dependency, so a future caller does not pass an unordered slice.📝 Proposed doc change
-/// Built by a single order-independent scan: every accumulation here is either -/// an idempotent insert or a `min`/`max`, so the tables do not depend on arrival -/// order. +/// Built by a single scan whose result does not depend on where a registration +/// sits relative to the events using it: lifespans fold with `min`/`max`, and +/// name lookups are filled before any of them is resolved. +/// +/// Conflicting registrations of the same key resolve last-writer-wins, so +/// [`Self::build`] must be given the timestamp-ordered slice produced by +/// `NvtxModelBuilder::build` for the outcome to stay deterministic.🤖 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/analyzer/src/tables.rs` around lines 80 - 84, Update the documentation above the pass-1 tables to remove the unconditional order-independence claim and explicitly state that deterministic results rely on build receiving a timestamp-ordered slice from NvtxModelBuilder::build, since several name tables use last-writer-wins updates.integrations/nvtx/analyzer/tests/fixtures/mod.rs (1)
40-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
span_idassert uniqueness likespan.
span_idreturns the first matching index.spanpanics when two spans share a name. If a fixture stream ever produces two spans with the same name, aparentassertion can pass against the wrong span. Reuse the same uniqueness rule for both helpers.♻️ Proposed refactor
pub fn span_id(model: &NvtxModel, name: &str) -> SpanId { - let index = model - .spans() - .iter() - .position(|span| span.name == name) - .unwrap_or_else(|| panic!("no span named {name:?}")); - SpanId(index) + let target = span(model, name); + let index = model + .spans() + .iter() + .position(|candidate| std::ptr::eq(candidate, target)) + .expect("the span came from this model"); + SpanId(index) }🤖 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/analyzer/tests/fixtures/mod.rs` around lines 40 - 47, Update span_id to enforce the same unique-name assertion as span: locate the matching span entries, panic when none or more than one matches, and return the SpanId only for the single match. Reuse the existing span helper’s uniqueness rule or matching logic rather than retaining first-match behavior.integrations/nvtx/analyzer/src/stats.rs (1)
98-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake a new
SpanKindfail to compile here.The
matches!filter excludes any futureSpanKindvariant silently.NvtxModelBuilder::buildinintegrations/nvtx/analyzer/src/model.rsstates the opposite convention: it lists ignored event variants explicitly so a new variant breaks the build. Use an exhaustivematchfor the same protection.♻️ Proposed refactor
- for span in spans - .iter() - .filter(|span| matches!(span.kind, SpanKind::PushPop | SpanKind::StartEnd)) - { + for span in spans.iter().filter(|span| match span.kind { + SpanKind::PushPop | SpanKind::StartEnd => true, + // Marks are instants and resource lifespans are not work. + SpanKind::Resource => false, + }) {🤖 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/analyzer/src/stats.rs` around lines 98 - 101, Replace the `matches!` predicate in the span iteration with an exhaustive `match` on `SpanKind`, returning true for `PushPop` and `StartEnd` and explicitly false for every currently ignored variant. Follow the explicit-ignored-variant convention used by `NvtxModelBuilder::build` so adding a new `SpanKind` variant causes a compile error.
🤖 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.
Nitpick comments:
In `@integrations/nvtx/analyzer/src/stats.rs`:
- Around line 98-101: Replace the `matches!` predicate in the span iteration
with an exhaustive `match` on `SpanKind`, returning true for `PushPop` and
`StartEnd` and explicitly false for every currently ignored variant. Follow the
explicit-ignored-variant convention used by `NvtxModelBuilder::build` so adding
a new `SpanKind` variant causes a compile error.
In `@integrations/nvtx/analyzer/src/tables.rs`:
- Around line 80-84: Update the documentation above the pass-1 tables to remove
the unconditional order-independence claim and explicitly state that
deterministic results rely on build receiving a timestamp-ordered slice from
NvtxModelBuilder::build, since several name tables use last-writer-wins updates.
In `@integrations/nvtx/analyzer/tests/fixtures/mod.rs`:
- Around line 40-47: Update span_id to enforce the same unique-name assertion as
span: locate the matching span entries, panic when none or more than one
matches, and return the SpanId only for the single match. Reuse the existing
span helper’s uniqueness rule or matching logic rather than retaining
first-match behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 6b0cac9d-c25e-4a7e-b3b1-07a57ee926df
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (16)
Cargo.tomlintegrations/nvtx/analyzer/Cargo.tomlintegrations/nvtx/analyzer/src/lib.rsintegrations/nvtx/analyzer/src/model.rsintegrations/nvtx/analyzer/src/ranges.rsintegrations/nvtx/analyzer/src/resource.rsintegrations/nvtx/analyzer/src/span.rsintegrations/nvtx/analyzer/src/stats.rsintegrations/nvtx/analyzer/src/tables.rsintegrations/nvtx/analyzer/tests/fixtures/mod.rsintegrations/nvtx/analyzer/tests/pushpop.rsintegrations/nvtx/analyzer/tests/reconstruction.rsintegrations/nvtx/analyzer/tests/resolution.rsintegrations/nvtx/analyzer/tests/resource.rsintegrations/nvtx/analyzer/tests/roundtrip.rsintegrations/nvtx/analyzer/tests/stats.rs
| // The domain on a `RangeEnd` is redundant: `range_id` is | ||
| // process-globally unique, so it alone identifies the range. |
There was a problem hiding this comment.
One thought about process-global uniqueness. Quent is designed to work for distributed systems too. If multiple processes are using NVTX, we need to ensure all NVTX data is properly attributed to that scope. This could be solved at a higher layer than what NvtxModel will provide so this PR doesn't necessarily need changes right now, but it is food for thought.
For our immediate purpose of instrumenting query engines there's already a "worker" entity that is supposed to represent a "process". One thought here is to represent the entire injection library as an "entity" so it gets its own unique id, and it would have two events:
- one once-event with an entity reference to the entity representing the process (e.g. in the query engine case the worker entity).
- one multi-event to push nvtx events
This way in QE analysis we can construct one NvtxModel per "worker".
Opened this issue: #522 to address external sources referencing OS id-ed things in general
There was a problem hiding this comment.
Agreed, leaving as-is — one NvtxModel covers one process's id spaces, so a distributed capture builds one per worker. Thanks for filing #522.
| "{} nvtx resource(s) were never destroyed; closing them at trace end ({trace_end})", | ||
| leaked.len() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Same — drain_unclosed, end: None. A handle recreated while still open now also ends None, which reads like any other missing close, so the reuse is counted in ReconstructionAnomalies::reused_resource_handles (and reused_range_ids for the range equivalent).
644ea7a to
83893f4
Compare
Reconstructs a captured NVTX event stream into an in-memory model of spans, marks, domains, threads and categories. Off the shared analysis framework, which is what keeps zero-duration and never-closed spans representable. Two passes: order by timestamp and learn every handle registration, then replay against it. Arrival order and forward references stop mattering. Correlation keys, each chosen because the alternative fails silently rather than loudly: `range_id` alone for RangeStart/End, a per-(thread, domain) stack for Push/Pop, the bare handle for resources since the destroy carries no domain, per-domain keying for registered strings and category names. Incomplete pairs are represented, not guessed, and nothing is substituted for what the stream never said. An open with no close keeps everything the open stated and ends `None`, leaving it to the consuming analysis to decide whether to bound it and at what. A close with no open yields no span at all — the closing events carry only a correlation key, with no name or attributes to build one from — and is counted in `ReconstructionAnomalies`, along with any key reused while still open. Range statistics therefore cover observed closes only. Kind-conditional fields live in `SpanKind` variant data: the thread and parent on `PushPop`, the identifier type on `Resource`. A start/end range with a thread, or a resource with a parent, is unrepresentable rather than merely undocumented. Trace bounds are retained on the model, for a consumer that wants a right edge for a span that has none. The largest span end is not a substitute, since the last event may carry no span at all. Tested against hand-built streams, plus a feature-gated roundtrip over a real in-process capture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
65eba4a to
52f4403
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
integrations/nvtx/analyzer/src/tables.rs (1)
225-235: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider including the domain in the unregistered-string placeholder.
unregistered_string_namerenders only the handle.unnamed_category_namerenders the domain and the id. Registered strings key on(domain, handle), as the module docs state at lines 15-17. Two different domains that reference the same unregistered handle therefore produce identical labels. Adding the domain keeps the placeholder policy consistent with the keying rule.♻️ Proposed change
-/// Placeholder for a registered-string handle that was never registered. -fn unregistered_string_name(handle: u64) -> String { - format!("<unregistered string 0x{handle:X}>") -} +/// Placeholder for a registered-string handle that was never registered. +/// +/// The domain is in the label because registered strings key on +/// `(domain, handle)`. +fn unregistered_string_name(domain: u64, handle: u64) -> String { + format!("<unregistered string 0x{handle:X} @ domain 0x{domain:X}>") +}- .unwrap_or_else(|| unregistered_string_name(*handle)), + .unwrap_or_else(|| unregistered_string_name(domain, *handle)),Note:
tests/resolution.rsline 198 asserts the current label, so update that assertion too.🤖 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/analyzer/src/tables.rs` around lines 225 - 235, Update resolve_message for unregistered RegisteredHandle values to generate placeholders containing both domain and handle, consistent with the (domain, handle) lookup key and unnamed_category_name; update the affected tests/resolution.rs assertion to expect the domain-inclusive label.
🤖 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.
Nitpick comments:
In `@integrations/nvtx/analyzer/src/tables.rs`:
- Around line 225-235: Update resolve_message for unregistered RegisteredHandle
values to generate placeholders containing both domain and handle, consistent
with the (domain, handle) lookup key and unnamed_category_name; update the
affected tests/resolution.rs assertion to expect the domain-inclusive label.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: fa377211-de16-4d5b-9046-fdd3ef8b3576
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (17)
Cargo.tomlintegrations/nvtx/analyzer/Cargo.tomlintegrations/nvtx/analyzer/src/anomalies.rsintegrations/nvtx/analyzer/src/lib.rsintegrations/nvtx/analyzer/src/model.rsintegrations/nvtx/analyzer/src/ranges.rsintegrations/nvtx/analyzer/src/resource.rsintegrations/nvtx/analyzer/src/span.rsintegrations/nvtx/analyzer/src/stats.rsintegrations/nvtx/analyzer/src/tables.rsintegrations/nvtx/analyzer/tests/fixtures/mod.rsintegrations/nvtx/analyzer/tests/pushpop.rsintegrations/nvtx/analyzer/tests/reconstruction.rsintegrations/nvtx/analyzer/tests/resolution.rsintegrations/nvtx/analyzer/tests/resource.rsintegrations/nvtx/analyzer/tests/roundtrip.rsintegrations/nvtx/analyzer/tests/stats.rs
`nvtxDomainMarkEx` reports no thread, so `NvtxMark::thread_id` was set to `None` at its one construction site and read nowhere. An `Option` that is structurally never `Some` invites a consumer to branch on it. The same rule the span kinds now follow: a field that cannot be populated does not exist, rather than existing and being documented as empty. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
|
/merge |
2 participants
Summary
Closes #372. Adds
integrations/nvtx/analyzer— a framework-free reconstruction core turning a capturedEvent<NvtxEventEntity>stream into a labeledNvtxModel. No dependency on any legacy macros.Two passes. The first orders by timestamp and learns every handle registration; the second replays against those tables. By then neither arrival order nor forward references matter.
Four match keys, each chosen because the alternatives fail silently rather than loudly:
RangeStart/RangeEndrange_idaloneRangePush/RangePop(thread_id, domain)Kind-conditional fields live in
SpanKindvariant data — thread and parent onPushPop, identifier type onResource— so a start/end range with a thread is unrepresentable rather than merely undocumented.Tolerance. Nothing is substituted for what the stream never said. An open with no close keeps everything the open stated and ends
None, as does one whose key was reused while still open. A close with no open yields no span — the closing events carry only a correlation key, with no name or attributes to build one from. Both that and the reuse are counted inReconstructionAnomalies. Drains sort for determinism, arithmetic is saturating or checked throughout, and unresolved handles get placeholders that are pure functions of the raw id, so they cannot pass as real names.Statistics.
RangeStatsper(name, domain, category)— count and total/avg/min/max duration. Marks have no duration and resource lifespans measure existence rather than work, so neither participates; a span whose close was never captured raisescountwithout raisingobserved_count.Test plan
cargo test -p nvtx-analyzer— 33 tests, hermeticpixi run cargo test -p nvtx-analyzer --features real-capture-tests— adds the roundtrip over a real in-process capture (needs pixi for the nvtx-c headers)pixi run cargo clippy --workspace --all-targets --all-features --locked -- -D warningspixi run cargo fmt --all -- --checkGenerated with Claude Code