feat(nvtx): add quent-nvtx-injection cdylib (separable NVTX FFI) - #391
Conversation
|
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:
📝 WalkthroughWalkthroughAdds ChangesNVTX injection
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 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: 2
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
integrations/nvtx/injection/build.rs-11-12 (1)
11-12: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrack the header-source override as a build-script input.
Changing
NVTX_INCLUDE_DIRdoes not rerun this script, so a subsequent regeneration can silently retain bindings produced from the prior header tree.Proposed fix
fn main() -> Result<(), Box<dyn std::error::Error>> { println!("cargo::rerun-if-changed=wrapper.h"); + println!("cargo:rerun-if-env-changed=NVTX_INCLUDE_DIR");🤖 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/build.rs` around lines 11 - 12, Update the build-script main function to declare NVTX_INCLUDE_DIR as a Cargo rerun input alongside the existing wrapper.h directive, so changing the header-source override triggers regeneration of the bindings.integrations/nvtx/injection/src/init.rs-70-79 (1)
70-79: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn
-1for unbalanced pops
NVTXnvtxDomainRangePopreports errors with a negative value; this path currently returns0, which makes an unbalanced pop look like a successful pop of level zero.🤖 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 70 - 79, Update range_pop_level to return -1 when the domain’s depth is already zero before decrementing, matching NVTX unbalanced-pop semantics; otherwise decrement the depth and return the resulting 0-based level.integrations/nvtx/injection/src/convert.rs-370-389 (1)
370-389: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat
NVTX_MESSAGE_UNKNOWNas no messageNVTX_MESSAGE_UNKNOWNis the default unused state, so the wildcard branch warns on ordinary events with no message. ReturnNonefor that tag and keep the warning forNVTX_MESSAGE_TYPE_UNICODEand future encodings.🤖 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/convert.rs` around lines 370 - 389, Update decode_message to handle NVTX_MESSAGE_UNKNOWN explicitly by returning None without calling warn_unsupported_message_once. Keep the warning behavior in the wildcard branch for NVTX_MESSAGE_TYPE_UNICODE and other unsupported or future message encodings.
🧹 Nitpick comments (1)
integrations/nvtx/injection/src/init.rs (1)
231-334: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep the NVTX callback ABI types in one place.
integrations/nvtx/injection/src/init.rsandintegrations/nvtx/injection/src/callbacks.rsstill handwrite the same callback shapes, including the void-returning callbacks andDomainResourceCreate’s resource-attribute pointer. A shared alias layer would keep the Rust surface aligned with the NVTX typedefs and reduce drift.🤖 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 231 - 334, The callback ABI signatures are duplicated across the subscription registrations and callback implementations. Add a shared alias layer for all NVTX callback shapes, including void-returning callbacks and DomainResourceCreate’s resource-attribute pointer, then update the subscribe! registrations in integrations/nvtx/injection/src/init.rs lines 231-334 and the callback definitions in integrations/nvtx/injection/src/callbacks.rs lines 62-194 to reuse those aliases, keeping the signatures ABI-compatible with the NVTX typedefs.
🤖 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/injection/src/callbacks.rs`:
- Around line 22-193: Add focused tests for the callback functions in
callbacks.rs, covering ABI-compatible signatures, panic containment, null
attr/name/string handling, returned handles and nesting levels, and dispatch
behavior without panics escaping the extern "C" boundary. Also test that the
callback table installs these callbacks correctly, reusing existing
conversion/nesting test helpers and the visible on_* symbols rather than adding
unrelated coverage.
In `@integrations/nvtx/injection/src/init.rs`:
- Around line 121-125: Update InitializeInjectionNvtx2 to be declared pub unsafe
extern "C" fn, preserving its C ABI while requiring Rust callers to uphold the
documented get_export_table contract; keep the existing export and reexport
unchanged.
---
Other comments:
In `@integrations/nvtx/injection/build.rs`:
- Around line 11-12: Update the build-script main function to declare
NVTX_INCLUDE_DIR as a Cargo rerun input alongside the existing wrapper.h
directive, so changing the header-source override triggers regeneration of the
bindings.
In `@integrations/nvtx/injection/src/convert.rs`:
- Around line 370-389: Update decode_message to handle NVTX_MESSAGE_UNKNOWN
explicitly by returning None without calling warn_unsupported_message_once. Keep
the warning behavior in the wildcard branch for NVTX_MESSAGE_TYPE_UNICODE and
other unsupported or future message encodings.
In `@integrations/nvtx/injection/src/init.rs`:
- Around line 70-79: Update range_pop_level to return -1 when the domain’s depth
is already zero before decrementing, matching NVTX unbalanced-pop semantics;
otherwise decrement the depth and return the resulting 0-based level.
---
Nitpick comments:
In `@integrations/nvtx/injection/src/init.rs`:
- Around line 231-334: The callback ABI signatures are duplicated across the
subscription registrations and callback implementations. Add a shared alias
layer for all NVTX callback shapes, including void-returning callbacks and
DomainResourceCreate’s resource-attribute pointer, then update the subscribe!
registrations in integrations/nvtx/injection/src/init.rs lines 231-334 and the
callback definitions in integrations/nvtx/injection/src/callbacks.rs lines
62-194 to reuse those aliases, keeping the signatures ABI-compatible with the
NVTX typedefs.
🪄 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: 9e24f206-807a-43d4-9b0f-9df1e300a6e9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (11)
Cargo.tomldeny.tomlintegrations/nvtx/injection/Cargo.tomlintegrations/nvtx/injection/build.rsintegrations/nvtx/injection/c/symbol.cintegrations/nvtx/injection/src/bindings.rsintegrations/nvtx/injection/src/callbacks.rsintegrations/nvtx/injection/src/convert.rsintegrations/nvtx/injection/src/init.rsintegrations/nvtx/injection/src/lib.rsintegrations/nvtx/injection/wrapper.h
| | `quent-nvtx-events` | `events/` | The verbatim, Quent-agnostic NVTX event **vocabulary** (`NvtxEvent` + attribute/payload types). Pure Rust — no C, no NVTX headers, and nothing Quent-internal — so it can be offered upstream to the NVTX Rust crates later. | | ||
| | `quent-nvtx-injection` | `injection/` | The Quent-agnostic injection **cdylib** NVTX loads. Exports `InitializeInjectionNvtx2`, fills the NVTX callback tables, converts each call into a verbatim `NvtxEvent`, and hands it to a sink-agnostic `Fn(NvtxEvent)` hook. This is the only crate bound to the NVTX C ABI. | |
There was a problem hiding this comment.
Nit: if these are Quent-agnostic, we should maybe remove the quent- prefix?
There was a problem hiding this comment.
Let me handle that rename in a follow up PR which purely rename only so that its easy on review.
62bfae2 to
29b5895
Compare
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 (4)
Cargo.toml-65-65 (1)
65-65: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMove the YAML workspace change to a separate PR.
This is unrelated to the NVTX injection objective and expands the review scope. As per coding guidelines, “Keep each pull request focused on a single concern tied to its issue; submit unrelated fixes separately.”
🤖 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 `@Cargo.toml` at line 65, Remove the "crates/yaml" entry from the workspace configuration in Cargo.toml, leaving the NVTX injection changes focused and unchanged.Source: Coding guidelines
integrations/nvtx/README.md-67-68 (1)
67-68: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the default-build description.
build.rsdoes not includebindings.rs;src/lib.rscompiles it throughmod bindings;. The default build script does nothing.🤖 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/README.md` around lines 67 - 68, Update the README’s default and CI build description to state that build.rs does nothing by default and that src/lib.rs compiles the committed bindings.rs through mod bindings;, removing the incorrect claim that build.rs include!s the file.integrations/nvtx/injection/build.rs-14-18 (1)
14-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrack
NVTX_INCLUDE_DIRas a build input. Cargo won’t rerun this script when the override changes, so regenerated bindings can stay stale.Proposed fix
#[cfg(feature = "regenerate-bindings")] println!("cargo::rerun-if-changed=wrapper.h"); +#[cfg(feature = "regenerate-bindings")] +println!("cargo::rerun-if-env-changed=NVTX_INCLUDE_DIR");🤖 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/build.rs` around lines 14 - 18, Update the regenerate-bindings build-script configuration alongside the existing wrapper.h rerun directive to emit a Cargo rerun-if-env-changed directive for NVTX_INCLUDE_DIR, ensuring changes to that override trigger regenerate_bindings().integrations/nvtx/injection/src/convert.rs-370-409 (1)
370-409: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle
NVTX_MESSAGE_UNKNOWNexplicitly
NVTX_MESSAGE_UNKNOWNis the zero/default tag, so the_arm currently treats ordinary no-message attributes as unsupported and emits the warning. Add aNVTX_MESSAGE_UNKNOWN => Nonearm and keep the warning for actual unsupported encodings only.🤖 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/convert.rs` around lines 370 - 409, Update decode_message to match NVTX_MESSAGE_UNKNOWN explicitly and return None without invoking warn_unsupported_message_once. Keep the warning in the fallback arm so only genuinely unsupported message encodings emit the diagnostic.
🤖 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/injection/src/init.rs`:
- Around line 106-111: Update dispatch to use a per-thread reentrancy guard
before invoking the installed hook, returning without dispatching when the
current thread is already inside the callback. Ensure the guard resets reliably
during normal returns and unwinding, while preserving event dropping when no
hook is installed.
- Around line 73-79: Update range_pop_level to remove the domain entry from the
thread-local map when its depth reaches zero, rather than retaining a
zero-valued entry; avoid creating a new map entry for an unbalanced pop of an
unseen domain, and preserve the returned depth behavior for existing
positive-depth domains.
- Around line 162-172: Update integrations/nvtx/injection/src/init.rs lines
162-172 to emit the partial-capture diagnostic during initialization or callback
installation, rather than through message decoding. Update
integrations/nvtx/injection/src/convert.rs lines 370-409 so decode_message
returns None silently for NVTX_MESSAGE_UNKNOWN and warns only for unsupported
encodings.
- Around line 124-132: Make the initialization path centered on
InitializeInjectionNvtx2 non-panicking by preventing the diagnostic eprintln! in
install_callbacks from propagating a panic, and add an appropriate panic
boundary around the extern "C" entry point if needed. Ensure failures after
CORE2 installation are converted into the existing unsuccessful result while
preserving INITIALIZED state and retry behavior.
---
Other comments:
In `@Cargo.toml`:
- Line 65: Remove the "crates/yaml" entry from the workspace configuration in
Cargo.toml, leaving the NVTX injection changes focused and unchanged.
In `@integrations/nvtx/injection/build.rs`:
- Around line 14-18: Update the regenerate-bindings build-script configuration
alongside the existing wrapper.h rerun directive to emit a Cargo
rerun-if-env-changed directive for NVTX_INCLUDE_DIR, ensuring changes to that
override trigger regenerate_bindings().
In `@integrations/nvtx/injection/src/convert.rs`:
- Around line 370-409: Update decode_message to match NVTX_MESSAGE_UNKNOWN
explicitly and return None without invoking warn_unsupported_message_once. Keep
the warning in the fallback arm so only genuinely unsupported message encodings
emit the diagnostic.
In `@integrations/nvtx/README.md`:
- Around line 67-68: Update the README’s default and CI build description to
state that build.rs does nothing by default and that src/lib.rs compiles the
committed bindings.rs through mod bindings;, removing the incorrect claim that
build.rs include!s the file.
🪄 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: ee80b497-7d0e-4857-816f-0a75ac7d034e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (12)
Cargo.tomldeny.tomlintegrations/nvtx/README.mdintegrations/nvtx/injection/Cargo.tomlintegrations/nvtx/injection/build.rsintegrations/nvtx/injection/c/symbol.cintegrations/nvtx/injection/src/bindings.rsintegrations/nvtx/injection/src/callbacks.rsintegrations/nvtx/injection/src/convert.rsintegrations/nvtx/injection/src/init.rsintegrations/nvtx/injection/src/lib.rsintegrations/nvtx/injection/wrapper.h
| // any `--all-features` build/clippy (e.g. CI). Regeneration is a | ||
| // manual maintenance action that needs the NVTX headers (via | ||
| // `NVTX_INCLUDE_DIR` or the `nvidia-nvtx` git-dep). When they are |
There was a problem hiding this comment.
If we can avoid manual regeneration and checking in generated sources, we should. I think we can pin the NVTX headers (nvtx-c) and libclang in Pixi, then have build.rs always generate the bindings into OUT_DIR. This keeps the bindings synchronized with the headers without modifying tracked sources and we wouldn't need all the feature flag machinery here.
There was a problem hiding this comment.
Committing the generated bindings is deliberate — default and CI builds stay hermetic (no libclang toolchain or NVTX headers needed just to compile the crate). It also keeps the crate self-contained, which is exactly what we want since these crates are slated to be upstreamed (to nvtx-sys) and then removed from quent: a checked-in artifact lifts cleanly to the upstream home and leaves no libclang/header build dependency behind in quent's toolchain when we drop them. Generating into OUT_DIR would instead make libclang + NVTX headers a hard build requirement for every contributor, CI job, and downstream consumer.
There was a problem hiding this comment.
Bindgen via build.rs is what nvtx-sys does, so we'll have to add those dependencies when we move to that anyways?
And not all downstream consumers will use this crate.
There was a problem hiding this comment.
Committing the generated bindings is deliberate — default and CI builds stay hermetic (no libclang toolchain or NVTX headers needed just to compile the crate).
Why is this a problem when our CI builds through Pixi?
It also keeps the crate self-contained, which is exactly what we want since these crates are slated to be upstreamed (to nvtx-sys) and then removed from quent
This is another reason why it was previously suggested to create a personal fork of NVTX.
a checked-in artifact lifts cleanly to the upstream home and leaves no libclang/header build dependency behind in quent's toolchain when we drop them.
This seems strange, why is removing two lines of pixi toml so hard when we already have to remove hundreds of lines of generated code when we migrate?
Generating into OUT_DIR would instead make libclang + NVTX headers a hard build requirement for every contributor, CI job, and downstream consumer.
We have exactly one supported build environment which is Pixi based.
There was a problem hiding this comment.
A bit of context on why it started as committed bindings: the goal was hermetic, reproducible builds — no libclang toolchain or NVTX header fetch needed just to compile the crate — plus keeping it self-contained for the eventual upstream move. I considered that to be a safe approach for now before the dust settles down on this feature and I have to touch pixi for this.
I personally favor the above approach. I am fine with autogen approach too.
With libclang + nvtx-c pinned in pixi, bindings always track the pinned headers, there's no checked-in generated source to drift, and it lets us drop the regenerate-bindings.
I'll switch build.rs to generate into OUT_DIR which will also fold the manual abi types into the allowlist.
I would strongly recommend against using personal forks as a dependency — for known reasons that's discouraged in open-source projects, and we already pin official NVTX and an alternative approach, so we don't need one.
There was a problem hiding this comment.
Please let's reconsider forking and upstreaming instead, so we can skip all this work.
There was a problem hiding this comment.
There is an active issue for upstreaming effort already #388 I will pick that up once the phase 1 is complete while handling phase 2.
Personal forks is not a good approach, it creates a bottleneck with whoever creates the fork. I have seen this happen quite a lot to go that route again. If we handle upstreaming in parallel, this problem becomes obsolete, hopefully soon.
A Quent-agnostic NVTX injection cdylib: NVTX loads it via NVTX_INJECTION64_PATH, calls the exported InitializeInjectionNvtx2 to install the CORE/CORE2 callback tables, and each callback converts its arguments into a verbatim NvtxEvent handed to a sink-agnostic Fn(NvtxEvent) hook. Depends only on quent-nvtx-events, so it stays separable and upstreamable. Bindings are generated by bindgen in build.rs into OUT_DIR from the NVTX headers and libclang pinned in pixi (nvtx-c / libclang, linux-64); nothing is checked in. Conversion honors each attribute struct's declared size to avoid over-reads, copies immediate strings in before returning, and contains panics at the C boundary. Linux 64-bit only; integrations are opt-in (not in default-members). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
NVTX calls InitializeInjectionNvtx2 once per NVTX-using image in the process (the executable and each instrumented shared library keep their own per-image nvtxGlobals and initialize lazily), each passing its own export-table accessor. The previous OnceLock gate ran install_callbacks only for the first caller and returned the cached success to the rest, so later images' tables were never populated — NVTX then turned their functions into silent no-ops and their events were dropped. Install callbacks on every call instead. install_callbacks writes only into the caller-supplied tables and touches no shared capture state, so per-image (and concurrent or repeat) calls are independent and safe. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
The classic, non-domain-scoped NVTX API (nvtxMarkA, nvtxRangePushA, nvtxRangePop, nvtxRangeStartA/Ex, nvtxRangeEnd, nvtxNameCategoryA) dispatches through the CORE table, not the domain-scoped CORE2 table this crate already subscribed. Previously only nvtxNameOsThreadA was installed in CORE, so an app that uses the default-domain API — the most common NVTX pattern — had its calls turned into silent no-ops by NVTX and produced an empty capture with no diagnostic. Subscribe the full default-domain ASCII CORE surface, captured verbatim on the default domain (0). Range nesting levels and start/end ids are synthesized exactly as for the domain surface, keyed by domain 0, so an app that reads NVTX's return values still observes faithful behavior. No vocabulary change: quent-nvtx-events already models the default domain as domain 0. Wide-char (*W / Unicode) variants are subscribed with warn-once stubs: Unicode capture stays deferred, but such a call now emits a one-time diagnostic and keeps range nesting/ids valid instead of silently no-op'ing. Adds unit tests for the new default-domain conversions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
read_message and read_resource read the pointer-sized message union out of
the foreign attribute struct before checking the message tag. For a
NVTX_MESSAGE_UNKNOWN tag ("no message") the union is absent, and a caller
that zero-sizes or leaves it uninitialized would still have those bytes
read (an uninitialized-read, in-bounds but indeterminate). The read value
was then discarded, so behavior was unaffected, but the reads were
gratuitous and inconsistent with the payload path, which already returns
early on NVTX_PAYLOAD_UNKNOWN before touching its union.
Guard both readers: return no message on NVTX_MESSAGE_UNKNOWN before
reading the union, only reading it for tags that carry a value. Adds
full-size regression tests for both the event and resource paths.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
The Ex callbacks (mark, range start/push, resource create) computed their side effect — bumping the per-thread nesting level or minting a range/ resource handle that NVTX returns to the app — and then early-returned on a null attribute pointer, dropping the event. That desynced the capture stream from the app-facing state: the push's level or the start's/resource's handle was handed back, but no RangePush/RangeStart/ResourceCreate event was emitted, so a later pop/end/destroy showed up unpaired. Move null handling into convert: read_attributes_or_empty and read_resource now yield empty attributes / empty identity for a null attr, so the event is always emitted (never dropped) and pairing stays balanced. The callbacks drop their now-redundant null-attr early returns. A null attr is API misuse by the app (NVTX requires a valid attr), so this is defensive, but the fix keeps the captured range/handle stream consistent regardless. Adds tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
install_core2 returned success unconditionally once it obtained the CORE2 table, regardless of whether the individual callbacks actually installed. set_callback silently no-ops a slot that is out of range for the table's reported size or whose slot pointer is null, so a mismatched/older NVTX could leave part of the domain surface unhooked while init still reported full success with no diagnostic. set_callback and the subscribe! macro now return whether each callback installed; install_core2 counts them and emits a one-time diagnostic naming how many of the domain callbacks were installed when any are missing. It still returns success on a partial install: un-installed slots are calls the running NVTX does not expose (nothing to capture there), and failing would make NVTX discard the injection entirely — but the gap is now visible. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
range_push_a_captures_message_on_default_domain added no unique coverage: the three *A converters (mark_a/range_push_a/range_start_a) are trivial wrappers over the same message_only_attributes helper, which is already exercised thoroughly by mark_a_captures_message_on_default_domain (message plus empty category/color/payload) and range_start_a_captures_id_and_message (the *A path plus its unique range-id passthrough). The dropped test only re-asserted the wrapper picks the RangePush variant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
|
/merge |
PR 3 of 3, completing NVTX capture on top of rapidsai#386 (quent-nvtx-events) and rapidsai#391 (quent-nvtx-injection). `quent-nvtx` is the bridge that adapts each captured NvtxEvent into Quent's event pipeline and builds the self-configuring capture cdylib NVTX loads via NVTX_INJECTION64_PATH. - rlib: fronts Quent's unbounded EventSender with a bounded, lock-free ring (drop-and-count on overflow, surfaced at teardown); a drain thread forwards the ring to an exporter. The injection hook stamps a capture timestamp and enqueues without blocking the app thread. - cdylib: an ELF .init_array constructor reads QUENT_NVTX_OUTPUT_DIR (+ an optional QUENT_NVTX_SESSION), builds an ndjson pipeline, and installs the hook in the same image whose callbacks NVTX invokes; a .fini_array destructor flushes at exit. NvtxEventEntity is a transparent newtype over NvtxEvent that implements the pipeline's quent_events::EntityEvent (NAME = "NvtxEvent"); the vocabulary crate stays Quent-agnostic, so the newtype adapts it here. The e2e feature builds a deterministic, self-contained NVTX emitter (nvtx_test_app, no GPU) and a subprocess harness that attaches the cdylib and asserts every core NVTX kind, the CORE payload union, cross-thread range pairing, and per-thread naming round-trip through ndjson. NVTX client headers come from the pixi-pinned nvtx-c package ($CONDA_PREFIX/include), matching the injection crate; no NVTX git dep. Registered in workspace members only (not default-members), Linux 64-bit only. Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
… e2e) PR 3 of 3, on rapidsai#386 (events) + rapidsai#391 (injection). Adapts each captured NvtxEvent into Quent's pipeline; the self-configuring cdylib NVTX loads via NVTX_INJECTION64_PATH. - Hook stamps each event and pushes to a bounded, lock-free ring (drop-and-count on overflow); a drain thread forwards to an ndjson exporter. - cdylib self-configures from QUENT_NVTX_OUTPUT_DIR at load, flushes at exit. - NvtxEventEntity: transparent newtype over NvtxEvent implementing EntityEvent. - e2e: one multi-threaded C++ producer (domain-scoped, mimics libcudf) run under the cdylib; asserts every core kind round-trips through ndjson. Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
… e2e) PR 3 of 3, on rapidsai#386 (events) + rapidsai#391 (injection). Adapts each captured NvtxEvent into Quent's pipeline; the self-configuring cdylib NVTX loads via NVTX_INJECTION64_PATH. - Hook stamps each event and pushes to a bounded, lock-free ring (drop-and-count on overflow); a drain thread forwards to an ndjson exporter. - cdylib self-configures from QUENT_NVTX_OUTPUT_DIR at load, flushes at exit. - NvtxEventEntity: transparent newtype over NvtxEvent implementing EntityEvent. - e2e: one multi-threaded C++ producer (domain-scoped, mimics libcudf) run under the cdylib; asserts every core kind round-trips through ndjson. Signed-off-by: Pradeep Garigipati <pgarigipati@nvidia.com>
Wires captured NVTX events into a Quent pipeline, completing the foundational NVTX stack (#386 `quent-nvtx-events`, #391 `quent-nvtx-injection`). ### What it is The application drives capture: it owns its Quent `Context` and exporter, annotates its code with the NVTX Rust API, and links `quent-nvtx-injection` with its `static-injection` feature so NVTX initializes injection in-process at the first NVTX call — no cdylib, no `NVTX_INJECTION64_PATH`. - **`quent-nvtx-bridge`** — `NvtxEventEntity`, a `#[serde(transparent)]` newtype over `NvtxEvent` implementing Quent's `EntityEvent` (the orphan-rule adapter; the only crate depending on Quent internals). - **`quent-instrumentation`** — adds `Observer::sender()`, a cloned `EventSender` so the `'static` injection hook can emit into an app-owned observer that still flushes on drop. - **`quent-nvtx-injection`** — `static-injection` links the strong-symbol shim with `+whole-archive`, so the strong `InitializeInjectionNvtx2` overrides NVTX's weak no-op. - **`quent-nvtx-example`** — runnable wiring plus its test. ### Using it ```rust let observer = ctx.block_on(async { ctx.observer::<NvtxEventEntity>(opts).await })?; let sender = observer.sender(); quent_nvtx_injection::install_hook(move |e| { sender.send(Event::new_now(session, NvtxEventEntity::from(e))); })?; // ... nvtx::mark!(...), nvtx::range!(...) ... drop(observer); // flush ``` ### Tests `cargo test -p quent-nvtx-example` runs the example against a temp dir and asserts every core NVTX kind round-trips through ndjson. No GPU. Linux 64-bit only. Authors: - Pradeep Garigipati (https://github.com/9prady9) Approvers: - Johan Peltenburg (https://github.com/johanpel) URL: #402
3 participants
quent-nvtx-injection— the NVTX injection cdylibPR 2 of 3, following #386 (
quent-nvtx-events, merged). The bridge + capture e2e follows this one.What it is
NVTX_INJECTION64_PATH. ExportsInitializeInjectionNvtx2, fills the CORE/CORE2 callback tables, and converts each callback into thequent-nvtx-eventsvocabulary verbatim (raw handles, no resolution/decoding).quent-nvtx-events. Linux 64-bit only (compile_error!elsewhere).Safety & correctness
extern "C"callback runs insidecatch_unwind; a panic never crosses the C ABI. Synthesized handles/ids and push/pop levels are returned correctly even if conversion panics.InitializeInjectionNvtx2once per NVTX-using image (exe + each instrumented.sohas its ownnvtxGlobals), so callbacks are installed on every call, not just the first — otherwise later images are silently no-op'd.size; the payload union is read at the tagged member's exact width (no uninitialized upper bytes).const char*messages are copied into ownedStrings before returning; registered strings keep only their raw handle.Generated bindings
build.rsrunsbindgenover the NVTX ABI into$OUT_DIRon every build — nothing checked in, nobindings.rsto keep in sync (resource/range-id types folded into the allowlist; no hand-writtenconvert::abi).libclangcome from the pixi-pinnednvtx-c/libclangpackages ([target.linux-64.dependencies]), sopixi run cargo …is hermetic (build.rsreadsCONDA_PREFIX). Bumping the captured NVTX version is a one-linenvtx-cbump — no regen feature, NVTX git dep, ordeny.tomlallow-git.Captured surface
nvtxMarkA/Ex,nvtxRangePushA/Ex,nvtxRangePop,nvtxRangeStartA/Ex/nvtxRangeEnd,nvtxNameCategoryA,nvtxNameOsThreadA— captured on the default domain (0). No vocabulary change:quent-nvtx-eventsalready models the default domain asdomain 0.*W/Unicode): subscribed with warn-once stubs — Unicode capture deferred, but such a call emits a one-time diagnostic and keeps range nesting/ids valid instead of silently no-op'ing.Tests
*Aconversions). No NVTX headers or GPU required beyond the pixi-pinnednvtx-c.Upstreaming
nvtx-sysbehind atoolsfeature (currently producer-only) — parallel track, not a blocker.