Remove ovstage host copies - #7157
Conversation
Greptile SummaryThe PR centralizes ovstage construction around the GPU_INCREMENTAL hierarchy model and replaces per-frame GPU-to-host transform and point copies with stream-ordered, zero-copy DLTensor writes.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code failure established. Stage creation is consistently centralized on one hierarchy model, while zero-copy buffers remain alive through synchronous operation waits and the tested particle path preserves the expected CUDA pointer and tensor layout. Important Files Changed
Sequence DiagramsequenceDiagram
participant Physics as Newton / PhysX state
participant Warp as Warp GPU stream
participant Renderer as OVRTXRenderer
participant Stage as ovstage
participant OVRTX as OVRTX
Physics->>Warp: Produce body and particle state
Renderer->>Warp: Build object and camera transforms
Renderer->>Stage: write_attribute(DLTensor, cuda_stream)
Warp-->>Stage: Stream-ordered buffer availability
Stage->>Stage: Update GPU_INCREMENTAL hierarchy
Renderer->>Stage: advance_write_floor
Renderer->>OVRTX: Render updated scene
Reviews (1): Last reviewed commit: "Remove ovstage host copies and use GPU_I..." | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The shared ovstage stage factory and stream-ordered zero-copy Warp writes are coherent, but the new DLTensor conversion path requires ovstage 0.1.1 while the permitted installation guidance still includes 0.1.0.
- Design and architecture: Centralizing stage creation in
isaaclab_ov._stageconsistently applies the process-wideGPU_INCREMENTALhierarchy model across OVRTX and OvPhysX. The remaining integration issue is ensuring the package dependency matches the ovstage behavior this architecture requires. - API: The private helper API does not introduce a public compatibility concern, and making ovstage unconditional is consistent with the stated dependency model. However, the supported ovstage version contract must raise its lower bound from 0.1.0 to 0.1.1 because trailing-axis lane folding and stream-ordered writes depend on 0.1.1 semantics.
- Implementation: The transform and particle producer paths correctly pass their Warp CUDA stream into
write_attribute, and the writes are awaited while parent buffers remain live. The concrete defect is thatxform_tensor_from_warpandpoints_tensor_from_warpuse behavior explicitly unavailable in ovstage 0.1.0 without a corresponding dependency-version update, allowing runtime failures for otherwise permitted installations.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| Returns: | ||
| A :class:`ovstage.DLTensor` with shape ``[N]`` and ``lanes=16``. | ||
| """ | ||
| return ovstage.make_dltensor(xforms, dtype=OVSTAGE_XFORM_DTYPE) |
There was a problem hiding this comment.
🟡 Warning · Api — Zero-copy writes need unpinned ovstage 0.1.1
These helpers rely on ovstage folding a DLPack producer's trailing axes into lanes=16/lanes=3, and callers pass write_attribute(cuda_stream=...). The code removed in this diff documented that ovstage 0.1.0 rejects exactly these writes and that the accepted install range is ovstage>=0.1.0,<0.2.0. No dependency metadata is updated here, so a permitted 0.1.0 resolution now fails at render time on every transform and points write. Raise the isaaclab_ov ovstage lower bound to 0.1.1.
| points: Warp array of shape ``[N]`` and dtype :class:`warp.vec3f`. | ||
|
|
||
| Returns: | ||
| A :class:`ovstage.DLTensor` with shape ``[N]`` and ``lanes=3``. |
There was a problem hiding this comment.
nit: return types Any do not match the documentation.
| is_array=False, | ||
| semantic=ovstage.AttributeSemantic.MATRIX, | ||
| cuda_stream=wp.get_stream(self._device).cuda_stream, |
There was a problem hiding this comment.
Can we review situation with streams in subsequent PR please? Some helpers would be useful, Instead of wp.get_stream() we store it as a filed and we do cuda_stream=self._stream?
Additionally we have synchronization issues :
- Rigid xforms (binding.map + wp.launch) - mplicit: write into mapped buffer on Warp’s current stream. No explicit cuda_stream to OVRTX.
- Particles (binding.write(..., ASYNC)) - Explicit: passes wp.get_stream(...).cuda_stream so OVRTX can GPU-wait.
- Cameras (wp.launch then wp.copy into map) - Same as xforms — assume mapped memory is consumed later somehow.
Additionally @pv-nvidia for vis.
There was a problem hiding this comment.
I will address this in a tiny separate PR that can be merged into the release.
Note that because of a particular implementation detail in ovrtx 0.4 (copy-back uses the default NULL stream which syncs against all blocking streams, which includes the warp stream), it seems the current code is currently working fine without changes though
5f915a8 to
8b7d032
Compare
marcodiiga
left a comment
There was a problem hiding this comment.
Reviewed at 5a8972a9. The GPU lane folding, buffer lifetime, CPU_INCREMENTAL transform behavior, and non-default-stream round trips are sound. I found no P0 correctness issue, so I’m approving. I left two actionable follow-ups: a measured per-prim points-update performance regression and synchronization wording that currently overstates nonblocking behavior.
| particle_counts: Number of particles in each prim's slice. | ||
| """ | ||
| particle_slices = [ | ||
| _points_tensor_from_numpy(particle_np[particle_offset : particle_offset + particle_count]) | ||
| points_tensor_from_warp(particle_q[particle_offset : particle_offset + particle_count]) |
There was a problem hiding this comment.
This rebuilds a Warp slice and consumes a DLPack descriptor for every prim on every rendered frame. On an RTX A6000 with the repository-pinned OVStage 0.1.1.355824 and Warp 1.16.0, a real write_attribute(...).wait() benchmark with 128 points per prim measured 0.853 ms versus 0.265 ms for the previous host-staging path at 16 prims, 3.170 ms versus 0.603 ms at 64, and 12.255 ms versus 2.557 ms at 256. Reusing the descriptors reduced those measurements to 0.128, 0.329, and 1.126 ms. Please cache the per-query slices/descriptors and rebuild them only when the source pointer/device or offset/count layout changes, clearing the cache during teardown. A multi-prim regression benchmark would also cover this per-frame cost.
There was a problem hiding this comment.
I will ticket this up as a follow-up
| wp.synchronize_device(self._device) | ||
| # The tensor is handed over zero-copy, so ovstage reads ``object_transforms`` in place and | ||
| # must not do so until the kernel above has landed. ``cuda_stream`` gives it the Warp stream | ||
| # that kernel was enqueued on, so it inserts a GPU-side wait instead of us forcing a |
There was a problem hiding this comment.
cuda_stream provides producer ordering by draining work already queued on that stream before OVStage accesses the tensor; it does not promise insertion of a nonblocking GPU-side stream wait. Because this call immediately .wait()s for the write operation, the calling thread can still block; the improvement here is eliminating the host copy and replacing device-wide synchronization with stream-scoped producer ordering. Please update this comment, the analogous particle comment, and the changelog/PR description accordingly. If a nonblocking GPU-side handoff is required, use the event-based API and verify that path end to end.
There was a problem hiding this comment.
Thanks, addressed!
03156e6 to
6c86230
Compare
ovstage 0.1.1 fixes nvbug 6490020 so the OVRTX ovstage path no longer needs its per-frame host round-trip - Write transforms and points straight from their Warp GPU buffers; make_dltensor now folds a producer's trailing axes into the lane count omni:xform and points expect. - Order those writes with write_attribute(cuda_stream=...) instead of blocking the host on wp.synchronize_device, as the legacy binding path already does. - Drop the _OVSTAGE_AVAILABLE guard: ovstage is an unconditional dependency of isaaclab_ov, so the fallback was unreachable.
6c86230 to
91b0b4c
Compare
# Description ovstage 0.1.1 fixes nvbug 6490020 so the OVRTX ovstage path no longer needs its per-frame host round-trip - Write transforms and points straight from their Warp GPU buffers; make_dltensor now folds a producer's trailing axes into the lane count omni:xform and points expect. - Order those writes with write_attribute(cuda_stream=...) instead of blocking the host on wp.synchronize_device, as the legacy binding path already does. - Drop the _OVSTAGE_AVAILABLE guard: ovstage is an unconditional dependency of isaaclab_ov, so the fallback was unreachable. ## Type of change - New feature (non-breaking change which adds functionality) ## Checklist - [ ] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [ ] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I have added a changelog fragment under `source/<pkg>/changelog.d/` for every touched package (do **not** edit `CHANGELOG.rst` or bump `extension.toml` — CI handles that) - [ ] I have added my name to the `CONTRIBUTORS.md` or my name already exists there
## Description Bundled backport to `release/3.0.0` to reduce CI load. Source PRs reviewed for this bundle: - #7020 — already represented in `release/3.0.0`; its cherry-pick was empty, so no duplicate commit was added. - #7207 - #7229 - #7227 - #7231 - #6762 - #7208 - #7168 — backports the current PR head while the source PR is still open. - #7157 - #7216 ## Type of change - Bug fix - Documentation update - Workflow / packaging update ## Checklist - [x] I have read and understood the contribution guidelines. - [x] I have run formatting checks. - [x] Documentation changes are included. - [x] Documentation build generates no new warnings. - [x] Focused regression coverage passed. - [x] Required changelog fragments are included by the source PRs. - [x] Contributors are already listed or included by the source PRs. --------- Signed-off-by: Kelly Guo <kellyg@nvidia.com> Co-authored-by: Mustafa H <34825877+StafaH@users.noreply.github.com> Co-authored-by: Richard Lei <rilei@nvidia.com> Co-authored-by: Mustafa Haiderbhai <mhaiderbhai@nvidia.com> Co-authored-by: Kelly Guo <kellyg@nvidia.com>
Description
ovstage 0.1.1 fixes nvbug 6490020 so the OVRTX ovstage path no longer needs its per-frame host round-trip
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there