feat(nvtx): add UI viewport contracts - #562
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:
📝 WalkthroughWalkthroughThe PR adds the ChangesNVTX UI model layer
Estimated code review effort: 4 (Complex) | ~45 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: 2
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/ui/src/lib.rs-679-685 (1)
679-685: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
min_durationandmax_durationreport 0 when no range is complete.If every range in a group is incomplete,
accumulatereturns at Line 674 before it touchesmin_durationandmax_duration. Both fields stay at theirDefaultvalue of 0.finishthen emitsmin_duration: 0andmax_duration: 0.The doc comment at Lines 146-148 states that incomplete ranges never receive an inferred duration. A zero is an inferred duration to any consumer that does not first read
observed_count. The test at Lines 915-922 does not cover these two fields.The contract is new and unreleased. Change the two fields to
Option<u64>now, so the TypeScript type carriesnulland the Timeline UI cannot misread the value.🛡️ Proposed change
- pub min_duration: u64, - pub max_duration: u64, + pub min_duration: Option<u64>, + pub max_duration: Option<u64>,- min_duration: self.min_duration, - max_duration: self.max_duration, + min_duration: (self.observed_count > 0).then_some(self.min_duration), + max_duration: (self.observed_count > 0).then_some(self.max_duration),If you keep the
u64fields, add a test that asserts the values for an all-incomplete group, and document the sentinel on the struct.🤖 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/ui/src/lib.rs` around lines 679 - 685, Change the group summary’s min_duration and max_duration fields from u64 to Option<u64>, preserving None when accumulate receives only incomplete ranges and setting Some(duration) when the first complete range is observed. Update the initialization, min/max accumulation, finish serialization, and affected tests or TypeScript bindings so the resulting value is emitted as null rather than 0.
🧹 Nitpick comments (4)
integrations/nvtx/ui/src/lib.rs (4)
554-559: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the sorted-and-deduplicated invariant that
binary_searchdepends on.
selectedcallsbinary_searchonselection.category_ids. The call returns a wrong result if the vector is not sorted.canonicalize_requestsorts the vector at Line 341, andNvtxViewportResponse::from_modelalways canonicalizes at Line 374, so the invariant holds today.NvtxDomainSelection.category_idsis a public field, so a future entry point that skips canonicalization would silently drop ranges instead of failing.Add a doc comment that states the precondition.
♻️ Proposed doc comment
+/// Reports whether `category` is selected. +/// +/// # Preconditions +/// +/// `selection.category_ids` must be sorted. `NvtxCatalog::canonicalize_request` +/// establishes this invariant. fn selected(selection: &NvtxDomainSelection, category: Option<u32>) -> bool {🤖 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/ui/src/lib.rs` around lines 554 - 559, Add a doc comment directly above selected stating that selection.category_ids must be sorted and deduplicated before calling selected, because the category lookup uses binary_search. Keep the implementation unchanged.
819-869: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining
NvtxViewportErrorvariants.The test asserts
DuplicateDomainandEmptySelection. Four variants have no test:InvalidWindow,UnknownDomain,UnknownCategory, andUncategorizedUnavailable. The test name states that the canonical selection rules are enforced, so the gap is easy to miss in later changes.InvalidWindowmatters most, because every clipping computation inrange_itemandStatisticsAccumulator::accumulateassumesstart <= end.💚 Proposed additional assertions
let inverted = catalog.canonicalize_request(NvtxViewportRequest { viewport: NvtxViewportWindow { start: 250, end: 100, }, selections: vec![], }); assert!(matches!(inverted, Err(NvtxViewportError::InvalidWindow))); let unknown_domain = catalog.canonicalize_request(NvtxViewportRequest { viewport: canonical.viewport, selections: vec![NvtxDomainSelection { domain_id: 4242, category_ids: vec![3], include_uncategorized: false, }], }); assert!(matches!( unknown_domain, Err(NvtxViewportError::UnknownDomain { domain_id: 4242 }) )); let unknown_category = catalog.canonicalize_request(NvtxViewportRequest { viewport: canonical.viewport, selections: vec![NvtxDomainSelection { domain_id: 2, category_ids: vec![4242], include_uncategorized: false, }], }); assert!(matches!( unknown_category, Err(NvtxViewportError::UnknownCategory { category_id: 4242, .. }) ));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/ui/src/lib.rs` around lines 819 - 869, Extend canonical_selection_rules_are_enforced to assert all remaining NvtxViewportError variants: InvalidWindow for an inverted viewport, UnknownDomain for an unregistered domain, UnknownCategory for an unavailable category, and UncategorizedUnavailable using a domain without uncategorized support while requesting it. Keep the existing assertions unchanged and match each error variant with relevant field values where applicable.Source: Coding guidelines
413-422: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTwo
unreachable!calls panic on aSpanKind::Resourcevalue. Both matches rely on theis_rangefilter at Line 393, which is far from the match site. The shared root cause is that the "this span is a range" guarantee is carried by a separate call instead of by the type. If the analyzer later adds aSpanKindvariant thatis_rangeaccepts, or if a caller reachesrange_itemthrough another path, the server panics on a request thread instead of skipping the span.
integrations/nvtx/ui/src/lib.rs#L413-L422: replace theSpanKind::Resource { .. } => unreachable!(...)arm withcontinue, so an unexpected variant is skipped rather than fatal.integrations/nvtx/ui/src/lib.rs#L604-L608: makerange_itemreturnOption<NvtxRangeItem>and returnNonefor a non-range span, or move the range-kind resolution into a helper that returnsOption<NvtxRangeKind>and let the caller skip aNone.Either change removes the panic path while keeping the current behavior for every real input.
🤖 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/ui/src/lib.rs` around lines 413 - 422, Remove both panic paths for unexpected SpanKind::Resource values: in integrations/nvtx/ui/src/lib.rs lines 413-422, change the Resource arm in the span-kind match to skip the span with continue; in lines 604-608, update range_item to return Option<NvtxRangeItem> (or use an equivalent Option-returning range-kind helper) and have callers skip None. Preserve existing handling for valid range spans.
522-532: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse
domain_orderinstead of a linear search per statistics group.The closure runs
catalog.domains.iter().find(...)for every statistics group. The group count grows with the number of distinct message and category pairs. The cost is O(groups × domains).domain_orderat Line 516 already mapsdomain_idto an index.♻️ Proposed refactor
- let mut statistics = statistics + let mut statistics = statistics .into_iter() .map(|(key, accumulator)| { - let domain = catalog - .domains - .iter() - .find(|domain| domain.domain_id == key.domain_id) - .expect("statistics only include catalog domains"); + let index = *domain_order + .get(&key.domain_id) + .expect("statistics only include catalog domains"); + let domain = &catalog.domains[index]; accumulator.finish(&key, domain, model) }) .collect::<Vec<_>>();Move the
domain_orderconstruction above this block to apply the change.🤖 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/ui/src/lib.rs` around lines 522 - 532, Update the statistics transformation around the domain_order construction and the into_iter map to reuse domain_order for each key.domain_id instead of scanning catalog.domains with iter().find. Move or retain domain_order creation before this block, resolve the domain index through that map, and use it to access the corresponding catalog domain while preserving the existing expect behavior for unknown domains.
🤖 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/ui/src/lib.rs`:
- Around line 565-585: Update span_depths to memoize resolved parent-chain
depths and reuse them across spans, while retaining safe handling for missing
parents and cycles. Use the spans collection once, resolve each uncached chain
iteratively, and backfill cached depths from the chain. Add a test covering
several nested push/pop ranges and verifying the resulting depths.
- Around line 207-228: Refactor NvtxCatalog::from_model to build per-domain
thread IDs and uncategorized flags in single passes over model.spans() and
model.marks() before mapping model.domains(), then look up each domain by
domain.domain while preserving current categorization behavior. Avoid rescanning
spans or marks inside the domain closure; treat the optional catalog-caching
entry point for NvtxViewportResponse::from_model as a separate consideration
unless required by the existing design.
---
Other comments:
In `@integrations/nvtx/ui/src/lib.rs`:
- Around line 679-685: Change the group summary’s min_duration and max_duration
fields from u64 to Option<u64>, preserving None when accumulate receives only
incomplete ranges and setting Some(duration) when the first complete range is
observed. Update the initialization, min/max accumulation, finish serialization,
and affected tests or TypeScript bindings so the resulting value is emitted as
null rather than 0.
---
Nitpick comments:
In `@integrations/nvtx/ui/src/lib.rs`:
- Around line 554-559: Add a doc comment directly above selected stating that
selection.category_ids must be sorted and deduplicated before calling selected,
because the category lookup uses binary_search. Keep the implementation
unchanged.
- Around line 819-869: Extend canonical_selection_rules_are_enforced to assert
all remaining NvtxViewportError variants: InvalidWindow for an inverted
viewport, UnknownDomain for an unregistered domain, UnknownCategory for an
unavailable category, and UncategorizedUnavailable using a domain without
uncategorized support while requesting it. Keep the existing assertions
unchanged and match each error variant with relevant field values where
applicable.
- Around line 413-422: Remove both panic paths for unexpected SpanKind::Resource
values: in integrations/nvtx/ui/src/lib.rs lines 413-422, change the Resource
arm in the span-kind match to skip the span with continue; in lines 604-608,
update range_item to return Option<NvtxRangeItem> (or use an equivalent
Option-returning range-kind helper) and have callers skip None. Preserve
existing handling for valid range spans.
- Around line 522-532: Update the statistics transformation around the
domain_order construction and the into_iter map to reuse domain_order for each
key.domain_id instead of scanning catalog.domains with iter().find. Move or
retain domain_order creation before this block, resolve the domain index through
that map, and use it to access the corresponding catalog domain while preserving
the existing expect behavior for unknown domains.
🪄 Autofix
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: 2733b7b4-01d1-4e90-83b1-2f1f14b76436
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (3)
Cargo.tomlintegrations/nvtx/ui/Cargo.tomlintegrations/nvtx/ui/src/lib.rs
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/ui/src/lib.rs`:
- Around line 30-35: Update all six counter fields in
NvtxCatalogAnomalies—orphan_range_ends, orphan_range_pops,
orphan_resource_destroys, reused_range_ids, reused_resource_handles, and
total—to use decimal_u64 Serde serialization and TypeScript string types. Add
JSON and TypeScript declaration assertions covering u64::MAX for these fields.
🪄 Autofix
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: 3c1e0082-cb01-434b-ba7a-78ccf27f312f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (2)
integrations/nvtx/ui/Cargo.tomlintegrations/nvtx/ui/src/lib.rs
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/ui/src/lib.rs-676-678 (1)
676-678: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat an invalid parent reference as a root span.
When
parent_index >= spans.len(), Line 677 setsnext_depthto 1. The span then renders in a depth-1 lane even though no parent exists. Leavenext_depthat 0 for this case. Add a test for an out-of-rangeSpanId.Proposed fix
if index >= spans.len() { - next_depth = 1; break; }Also applies to: 701-705
🤖 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/ui/src/lib.rs` around lines 676 - 678, Update the invalid parent-reference handling in the span depth calculation to keep next_depth at 0 when the referenced index is out of range, so the span renders as a root. Apply the same change to the corresponding logic near the other reported location, and add a test covering an out-of-range SpanId.
🤖 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/ui/src/lib.rs`:
- Around line 676-678: Update the invalid parent-reference handling in the span
depth calculation to keep next_depth at 0 when the referenced index is out of
range, so the span renders as a root. Apply the same change to the corresponding
logic near the other reported location, and add a test covering an out-of-range
SpanId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: fd44f618-947a-40e8-b8f7-3cc479a9c610
📒 Files selected for processing (1)
integrations/nvtx/ui/src/lib.rs
9f7e2f7 to
ef01eed
Compare
Viewport contracts: purpose and data flowThe contracts in this PR form one pipeline:
In short:
Contract by contract:
Two implementation properties are important:
This separation keeps discovery, interaction state, and rendering payloads independent while preserving reconstruction fidelity and lossless timestamps. |
| /// Inclusive viewport bounds in Unix nanoseconds. | ||
| #[derive(TS, Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] | ||
| pub struct NvtxViewportWindow { | ||
| #[serde(with = "decimal_u64")] |
There was a problem hiding this comment.
javascript has to rely on bigint for nanosecond epoch timestamps, so we designed the resource timelines to give these start/end timestamps in decimal seconds relative to the query start time to get around that. If possible I would like to design NVTX viewport/range starts and ends to be the same.
Timeline Request:
Timeline response:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
integrations/nvtx/ui/src/lib.rs (2)
447-455: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCouple the cached catalog to its time origin.
from_model_with_catalogacceptscatalogandquery_startas independent parameters.NvtxCatalogalready storestrace_startandtrace_endrelative to the origin used when the catalog was built. If a caller caches a catalog and later passes a differentquery_start, the response times use one origin and the cached catalog uses another. No check detects the mismatch.Store the origin in the catalog and read it here, or document the invariant on the public function.
♻️ Option: derive the origin from the catalog
pub struct NvtxCatalog { + /// Absolute time origin used for every relative-second field. + #[serde(skip)] + #[ts(skip)] + pub query_start: TimeUnixNanoSec, /// Trace start in seconds relative to the query start. pub trace_start: f64,Then drop the
query_startparameter fromfrom_model_with_catalogand usecatalog.query_start.🤖 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/ui/src/lib.rs` around lines 447 - 455, Update NvtxCatalog and from_model_with_catalog so the catalog retains the query_start origin used during construction, then derive the viewport origin from catalog.query_start instead of accepting an independent query_start parameter. Update callers and construction paths to preserve this coupled origin and prevent cached catalogs from being used with mismatched time bases.
690-729: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the cycle and out-of-range parent branches.
The nested-chain test at Lines 1347-1405 exercises the normal path only. Lines 699-702 and Lines 707-714 handle an out-of-range parent index and a parent cycle. Neither branch has a test, and neither is reachable from
NvtxModelBuilderoutput.Note that an out-of-range parent yields depth 1, not depth 0. A child with an unresolvable parent therefore renders in a "depth 1" lane with no depth 0 lane above it. Confirm that this is the intended fallback, and pin it with a test.
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/ui/src/lib.rs` around lines 690 - 729, Add focused tests for the depth-calculation logic covering both an out-of-range parent index and a cyclic parent chain, using the relevant span-depth helper or test-accessible path rather than relying on NvtxModelBuilder output. Assert that an unresolvable parent assigns depth 1 and that cycle members receive depth 0, preserving the existing nested-chain behavior.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.
Inline comments:
In `@integrations/nvtx/ui/src/lib.rs`:
- Line 57: Update the response structs NvtxCatalogDomain, NvtxDomainLaneGroup,
NvtxRangeItem, NvtxMarkItem, and NvtxRangeStatistics so each domain_id field
uses decimal_u64 serialization and declares the TypeScript type as string,
matching NvtxDomainSelection. Extend the declaration test to verify domain_id:
string for all five response types.
---
Nitpick comments:
In `@integrations/nvtx/ui/src/lib.rs`:
- Around line 447-455: Update NvtxCatalog and from_model_with_catalog so the
catalog retains the query_start origin used during construction, then derive the
viewport origin from catalog.query_start instead of accepting an independent
query_start parameter. Update callers and construction paths to preserve this
coupled origin and prevent cached catalogs from being used with mismatched time
bases.
- Around line 690-729: Add focused tests for the depth-calculation logic
covering both an out-of-range parent index and a cyclic parent chain, using the
relevant span-depth helper or test-accessible path rather than relying on
NvtxModelBuilder output. Assert that an unresolvable parent assigns depth 1 and
that cycle members receive depth 0, preserving the existing nested-chain
behavior.
🪄 Autofix
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: 77577135-d747-47e0-8170-4ad849d609a8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!Cargo.lock
📒 Files selected for processing (2)
integrations/nvtx/ui/Cargo.tomlintegrations/nvtx/ui/src/lib.rs
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/ui/src/lib.rs-21-25 (1)
21-25: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve
query_startwhen restoringNvtxCatalog.
NvtxCatalogderivesDeserialize, so#[serde(skip)]restoresquery_startas0.from_model_with_catalogthen uses the wrong epoch and can return an empty viewport. RemoveDeserializeor require an explicitquery_start.🤖 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/ui/src/lib.rs` around lines 21 - 25, Update the NvtxCatalog deserialization contract around query_start: do not allow #[serde(skip)] to restore query_start as zero. Remove Deserialize from NvtxCatalog if catalogs are not meant to be restored, or require and deserialize an explicit query_start value so from_model_with_catalog always uses the original epoch.
🤖 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/ui/src/lib.rs`:
- Around line 21-25: Update the NvtxCatalog deserialization contract around
query_start: do not allow #[serde(skip)] to restore query_start as zero. Remove
Deserialize from NvtxCatalog if catalogs are not meant to be restored, or
require and deserialize an explicit query_start value so from_model_with_catalog
always uses the original epoch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 126fff72-93f7-42f8-8ee5-3e712ec2a31a
📒 Files selected for processing (2)
deny.tomlintegrations/nvtx/ui/src/lib.rs
joosthooz
left a comment
joosthooz
left a comment
There was a problem hiding this comment.
Just some small questions, I'm not sure if the serde thing is an actual problem
| #[serde(skip)] | ||
| #[ts(skip)] | ||
| query_start: TimeUnixNanoSec, |
There was a problem hiding this comment.
Why do we skip query_start, wouldn't that cause trouble when roundtripping data?
| ) -> Result<NvtxViewportRequest, NvtxViewportError> { | ||
| if !request.viewport.start.is_finite() | ||
| || !request.viewport.end.is_finite() | ||
| || request.viewport.start < 0.0 |
There was a problem hiding this comment.
Is it never possible for a trace to start before the query start, making this negative?
There was a problem hiding this comment.
From the way I understand these two, query start is the absolute 0.0 as per UI timeline (as per other telemetry info apart from NVTX events) for a given query, while the trace start is set to the minimum time ticker of the earliest/first nvtx event for this run; so I doubt trace_start can appear before query_start.
There was a problem hiding this comment.
Well, that is true only if my assumption that nvtx events are query scoped is true. We might have events before that also in the code. Let me think about this a bit more.
| .or_default() | ||
| .push(item); | ||
| } | ||
| SpanKind::StartEnd => domain_items.process_ranges.push(item), |
There was a problem hiding this comment.
Could this result in overlapping ranges, and is that a problem?
There was a problem hiding this comment.
yes, ranges are process wide and can definitely overlap depending on the how the nvtx range API calls are sprinkled around. Right now, data isn't altered but the current render will render rectangles on top of each other when there is a overlap.
It's not a bug but visually hard to grasp that - the only I can think of to resolve this to again stack them in that lane. @johallar what do you recommend here ?
I am not sure how much a visual cue will help though even if we try to solve this rendering overlap. The user can still get the actual time range info from the tooltip.
There was a problem hiding this comment.
Let's discuss tomorrow morning a bit, FE can do some intra-lane stacking or try to visualize overlapping spans in the same lane in some other way if i'm understanding correctly.
| threads.sort_by(|left, right| { | ||
| left.name | ||
| .cmp(&right.name) | ||
| .then(left.thread_id.cmp(&right.thread_id)) |
There was a problem hiding this comment.
Cosmetic nit; it looks like this would sort threads with default names like thread 1 .. thread 10 like
thread 1
thread 10
thread 2
...
There was a problem hiding this comment.
yeah, but there is no real ordering here to those threads than UI arrangement. Unless we want to put them top to bottom going from start time to end of the query time - perhaps that is useful didn' think of that to be honest until now.
|
/merge |
4 participants
Summary
Adds the Rust-owned NVTX exchange contract used by the server and Timeline UI.
The new
nvtx-uicrate:Related to #373
Verification