feat(nvtx): capture wide-char (*W) NVTX surface as UTF-8 - #540
Conversation
📝 WalkthroughWalkthroughWide-character NVTX messages now convert to UTF-8 and dispatch marker, range, category-name, and thread-name events. Wide range operations preserve synthesized IDs, nesting levels, thread IDs, and documented handling for unmatched operations. ChangesNVTX wide-character decoding
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Is there a reason we couldn't just support the wide char APIs, e.g. using: https://github.com/VoidStarKat/widestring-rs to convert? I have doubts about this work-around since the sentinel values clash with NVTX' spec (e.g. id uniqueness) and it adds complexity that doesn't need to exist if we were to simply support those APIs. |
Its not used anywhere in our current targets and it adds a lot more code which isn't used anywhere. Adding this when there is a consumer is easier with the tools we have now.
I didn't understand what you mean. Fix is to address the current behavior of no matching start/push for a end/pop, respectively. |
If the wide-character APIs are not used anywhere, what requires this workaround now?
Is the missing captured start/push event caused by something outside our control, or by our choice of not dispatching an event for the *W callbacks? |
Without the surface API to capture these, the push/pops incorrectly get hold of any other push/pop and/or silently don't say anything. We surface a warning when we encounter these by providing the entry points at least.
These are actual nvtx API calls out of our control right, handling them incorrectly is a bug in this crate not the other way around. |
Indeed, the bug is that we're not emitting events we're supposed to be emitting. Why is using sentinel values and an additional thread local stack to filter out events better than actually emitting those events? |
c4dc88c to
cd76b7b
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. |
9prady9
changed the title
You're right, I got it wrong. Updated the PR — |
cd76b7b to
24983c0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/callbacks.rs (1)
241-244: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftImplement event suppression for close events matching uncaptured push operations.
The documentation at lines 239–240 and 279–280 states that
RangeEndandRangePopevents closing pushes not captured by the injection layer must be dropped. The code currently dispatches these events unconditionally. The suppression functions referenced in the comments (init::UNDECODABLE_RANGE_IDandinit::pop_is_suppressed) do not exist.Add tracking to distinguish ranges and pops initiated by the application from those initiated by the injection layer:
integrations/nvtx/injection/src/callbacks.rs#L241–244: Implement a guard that dropsRangeEndevents for application-initiated range IDs before dispatch.integrations/nvtx/injection/src/callbacks.rs#L281–287: Implement a guard that dropsRangePopevents for application-initiated pops before dispatch.Preserve the nesting-level return values for faithful application-side semantics, but filter the dispatched events.
🤖 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/callbacks.rs` around lines 241 - 244, The callbacks must suppress dispatch of close events corresponding to application-initiated, uncaptured pushes while preserving the NVTX nesting-level return semantics. In integrations/nvtx/injection/src/callbacks.rs:241-244, update on_range_end to guard application range IDs using the shared undecodable-range tracking, dispatching only captured ranges; in integrations/nvtx/injection/src/callbacks.rs:281-287, update on_range_pop to use pop suppression tracking and dispatch only captured pops, while retaining each callback’s existing return value 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.
Outside diff comments:
In `@integrations/nvtx/injection/src/callbacks.rs`:
- Around line 241-244: The callbacks must suppress dispatch of close events
corresponding to application-initiated, uncaptured pushes while preserving the
NVTX nesting-level return semantics. In
integrations/nvtx/injection/src/callbacks.rs:241-244, update on_range_end to
guard application range IDs using the shared undecodable-range tracking,
dispatching only captured ranges; in
integrations/nvtx/injection/src/callbacks.rs:281-287, update on_range_pop to use
pop suppression tracking and dispatch only captured pops, while retaining each
callback’s existing return value behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: f734de27-368b-4340-983d-e73150081736
📒 Files selected for processing (3)
integrations/nvtx/injection/src/callbacks.rsintegrations/nvtx/injection/src/convert.rsintegrations/nvtx/injection/src/init.rs
|
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.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
integrations/nvtx/injection/src/callbacks.rs-238-240 (1)
238-240: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove stale suppression documentation.
The shown callbacks always construct and dispatch
RangeEndandRangePopevents. The documented sentinel and suppression paths are not present in this dispatch flow.
integrations/nvtx/injection/src/callbacks.rs#L238-L240: Remove the claim that ends usinginit::UNDECODABLE_RANGE_IDare dropped.integrations/nvtx/injection/src/callbacks.rs#L276-L280: Remove the claim thatinit::pop_is_suppresseddrops pops.🤖 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/callbacks.rs` around lines 238 - 240, Remove the stale suppression claims from the callback documentation: in integrations/nvtx/injection/src/callbacks.rs lines 238-240, remove the statement that init::UNDECODABLE_RANGE_ID ends are dropped; at lines 276-280, remove the statement that init::pop_is_suppressed drops pops. No dispatch logic changes are needed.
🤖 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.
Other comments:
In `@integrations/nvtx/injection/src/callbacks.rs`:
- Around line 238-240: Remove the stale suppression claims from the callback
documentation: in integrations/nvtx/injection/src/callbacks.rs lines 238-240,
remove the statement that init::UNDECODABLE_RANGE_ID ends are dropped; at lines
276-280, remove the statement that init::pop_is_suppressed drops pops. No
dispatch logic changes are needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 617e93e7-a944-4c3c-aeb7-75218587df18
📒 Files selected for processing (2)
integrations/nvtx/injection/src/callbacks.rsintegrations/nvtx/injection/src/convert.rs
On Linux wchar_t is 32-bit (UTF-32), so each code unit is a Unicode scalar value. copy_wchar iterates the NUL-terminated sequence and builds an owned UTF-8 String that enters the event stream as a plain NvtxMessage::String — identical to the ASCII surface from the downstream perspective. No changes to nvtx-events, the analyzer, or the bridge. Previously the *W callbacks were stubs that warned once and returned sentinel values / suppressed matching close events to avoid orphaning the stream. That approach violated the nvtxRangeId_t uniqueness contract (u64::MAX returned for every RangeStartW call) and added more machinery than it saved. Replacing the stubs with real implementations removes the suppression thread-local, the sentinel constant, and the guards in on_range_end / on_range_pop. The NVTX_MESSAGE_TYPE_UNICODE attribute path (domain-scoped *Ex calls with messageType=UNICODE) is also handled in decode_message alongside the existing ASCII and RegisteredHandle cases. Five new convert tests cover the wchar conversion directly: copy_wchar for ASCII, Unicode code points (U+1F600), and null; mark_w, range_push_w, range_start_w for the default-domain surface; and unicode_message_type_in_event_attributes_is_decoded for the domain-scoped attribute path. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
24983c0 to
eade0a2
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/injection/src/convert.rs (1)
1260-1282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for invalid UTF-32 scalar values.
copy_wcharreplaces invalid scalar values withchar::REPLACEMENT_CHARACTER, but the tests cover only valid values and null. Add a surrogate-value case to preserve this FFI conversion behavior.Proposed test
+ #[test] + fn copy_wchar_replaces_invalid_unicode_scalar_values() { + let wide = [0xD800 as libc::wchar_t, 0]; + // SAFETY: `wide` is a valid NUL-terminated wchar_t array. + let s = unsafe { copy_wchar_pub(wide.as_ptr()) }; + assert_eq!(s, "\u{FFFD}"); + }As per coding guidelines, “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/injection/src/convert.rs` around lines 1260 - 1282, Add a test alongside the existing copy_wchar tests that constructs a NUL-terminated wide string containing a surrogate code point, invokes copy_wchar_pub, and asserts the invalid scalar is converted to char::REPLACEMENT_CHARACTER while surrounding valid text remains intact.Source: Coding guidelines
🤖 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/injection/src/convert.rs`:
- Around line 1260-1282: Add a test alongside the existing copy_wchar tests that
constructs a NUL-terminated wide string containing a surrogate code point,
invokes copy_wchar_pub, and asserts the invalid scalar is converted to
char::REPLACEMENT_CHARACTER while surrounding valid text remains intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: a2855a84-67e9-4e9d-b002-fce5afd287e4
📒 Files selected for processing (2)
integrations/nvtx/injection/src/callbacks.rsintegrations/nvtx/injection/src/convert.rs
|
/merge |
2 participants
Summary
Implements the NVTX
*W(wide-char / Unicode) callback surface on Linux, wherewchar_tis 32-bit UTF-32. Each code unit maps directly to a Unicode scalar value viachar::from_u32; the result enters the event stream as a plainNvtxMessage::String— identical to the ASCII surface from the downstream perspective.copy_wchariterates a NUL-terminatedwchar_t*and builds an owned UTF-8String*Wcallbacks (MarkW,RangeStartW,RangePushW,NameCategoryW,NameOsThreadW) are now full implementations instead of warn-once stubsNVTX_MESSAGE_TYPE_UNICODEin event attributes (nvtxDomainRangePushExetc.) is handled indecode_messagealongside the existing ASCII and registered-string casesnvtx-injectionis touched —nvtx-events,nvtx-bridge, andnvtx-analyzerare unchanged since wide strings arrive asNvtxMessage::StringdownstreamTest plan
pixi run cargo test -p nvtx-injection— 30 tests pass (5 new:copy_wcharfor ASCII/Unicode/null,mark_w,range_push_w,range_start_w,unicode_message_type_in_event_attributes_is_decoded)pixi run cargo test -p nvtx-analyzer -p nvtx-example— all pass, no downstream changes🤖 Generated with Claude Code