fix: scale FabricFrameView selections to the view, not the stage - #6805
fix: scale FabricFrameView selections to the view, not the stage#6805pv-nvidia wants to merge 9 commits into
Conversation
1bd05de to
6787d38
Compare
deaadf7 to
21d3d22
Compare
91ba6fe to
10d4ff1
Compare
create_mapping resolved every input path with list.index, an O(N^2) scan that takes minutes at the ~200k rigid bodies of an 8192-env scene. Build a path -> output-index dict once (first occurrence wins, matching list.index semantics) and resolve each path in O(1). Adopted unchanged from PR isaac-sim#6554. Co-authored-by: yts-nv <yts-nv@users.noreply.github.com>
FabricFrameView selected prims by requiring only the Fabric world and local matrix attributes, which every xformable in the stage carries. Resolving the view's prims against that selection built a python path-to-index dict over ~1.1M prims on every environment reset. The allocation churn drove multi-second cyclic-GC stalls between rendered frames at high environment counts (nvbug 6535498); Kit-side per-frame work was unaffected, which is why the stall was invisible to Tracy. Tag each view's prims (and their parents) with per-view uint index attributes and require the tag in every selection, so selections match O(view) prims instead of O(stage). Rebuild the view-to-fabric slot mapping in a Warp kernel over the index attribute on each access: values travel with rows across bucket moves, so the mapping can never go stale and no cache or invalidation key is needed. Selections are guarded with GetCount(), which is exact because the per-view tag makes membership unambiguous. Attribute names embed a process-wide monotonic uid so a dead view's leftovers can never satisfy a live selection. Supersedes the fabric_frame_view half of PR isaac-sim#6554, whose cache keyed on selection length could silently serve stale indices after a same-count membership change or bucket reorder.
The view index attributes are authored as Fabric UInt and flow through the kernels as uint32, so the int32 slot arrays read as an unexplained inconsistency. They are not a style choice: Warp's check_index_array rejects any dtype other than int32 for indexed-array indices, so anything handed to wp.indexedfabricarray must be int32. Record that constraint where a reader meets it: the ArrayInt32_1d alias, both kernels that cross the boundary, and the buffer declarations in FabricFrameView.
Assert each selection matches exactly the prims the view manages rather than every prim on the stage. Without the per-view index attribute in the selection predicate the child selections pick up the parents too, so this fails with "matched 8 prims, expected 4".
10d4ff1 to
fa139eb
Compare
Greptile SummaryThis PR replaces stage-wide host path resolution with per-view Fabric index attributes and GPU-built slot mappings, while also making scene-data path mapping linear rather than quadratic.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code defect identified. The new mappings preserve existing path semantics, and the Fabric access paths rebuild scoped slot maps before use without exposing retained indexed arrays to subsequent buffer refreshes. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
V[FabricFrameView prim paths] --> T[Author per-view child and parent indices]
T --> S[Scoped Fabric selections]
S --> K[Warp kernels invert indices into Fabric slots]
K --> I[Indexed Fabric matrix arrays]
I --> P[Camera and frame pose reads]
B[Backend transform paths] --> D[First-occurrence reverse dictionary]
O[Requested output paths] --> D
D --> M[Linear transform mapping]
Reviews (1): Last reviewed commit: "Removed comment" | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The scoped Fabric selections and device-side slot mapping remove the whole-stage lookup bottleneck, while SceneDataProvider.create_mapping preserves first-occurrence semantics with linear-time construction. One maintainability and performance issue remains: each recreated view permanently leaves uniquely named Fabric index attributes on its prims.
- Design and architecture: Per-view tagged selections avoid host-side path resolution and stale mapping caches, but they introduce persistent per-view stage state without a lifecycle or reclamation mechanism. Repeated view creation therefore accumulates attributes and progressively increases initialization cost.
- API:
create_mappingretains its documented behavior: first occurrence wins, unmatched paths map to-1, and identity mappings returnNone. Removed FabricFrameView helpers and fields are private, and the in-repository test consumer was migrated. A cleanup API or an explicitly documented one-view-per-stage lifetime contract is needed for the new authored attributes. - Implementation: The child and parent tagging, scoped selections, count checks, and Warp mapping refresh paths are internally consistent. However,
_initialize_fabriccreates UID-suffixed attributes on managed prims and parents without any teardown path, so recreating views on a long-lived stage permanently widens Fabric state and measurably slows later initialization.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| rt_xformable.CreateFabricHierarchyLocalMatrixAttr() | ||
| rt_xformable.SetLocalXformFromUsd() | ||
| rt_xformable.SetWorldXformFromUsd() | ||
| rt_prim.CreateAttribute(index_attr, usdrt.Sdf.ValueTypeNames.UInt, custom=True) |
There was a problem hiding this comment.
🟡 Warning · Design Architecture — Per-view index attributes are never removed
_initialize_fabric authors two uid-suffixed uint attributes on every managed prim and unique parent, and no code path deletes them. Since the uid counter never reuses names, each view recreated over the same prims on a long-lived stage permanently adds another attribute pair, widening those prims' Fabric buckets and slowing subsequent view initialization. Add a teardown (explicit close/lifecycle hook) that drops this view's index attributes, or document the one-view-per-stage lifetime requirement.
| RuntimeError: If the prim is directly under the stage root and thus has | ||
| no non-pseudoroot parent to read Fabric matrices from. | ||
| """ | ||
| parent = prim_path.rsplit("/", 1)[0] |
There was a problem hiding this comment.
No need to split all parts, we just need the last part! This can be optimized
|
|
||
| # Unique parents in first-occurrence order; ``parent_ordinal`` maps a | ||
| # parent path to its position in that order. | ||
| self._unique_parent_paths = list(dict.fromkeys(_parent_path(p) for p in self.prim_paths)) |
There was a problem hiding this comment.
We should cache _parent_path(p) for p in self.prim_paths) because we call _parent_path again later
| self._rebuild_ro_arrays() | ||
| self._rebuild_rw_arrays() | ||
| self._child_parent_map = wp.array( | ||
| [parent_ordinal[_parent_path(p)] for p in self.prim_paths], dtype=wp.uint32, device=self._device |
There was a problem hiding this comment.
Here we should use the earlier cached parent paths
| # at the ~200k rigid bodies of an 8192-env scene. | ||
| path_to_out: dict[str | None, int] = {} | ||
| for out_idx, out_path in enumerate(paths): | ||
| if out_path not in path_to_out: |
There was a problem hiding this comment.
Do we need this check at all?
Two cases exists here:
- we have duplicate entries in the input_path. One of them will not be handled, which results in incorrect behavior either way
- we have None or invalid paths. That will result in negative indices and the kernels won't like that, they will crash or give undefined behavior.
So it doesn't matter what we do, pick the first or last, we will get incorrect behavior anyway. So we better pick the most performant form. So could be just a dict comprehension
Address review feedback on isaac-sim#6805: - _parent_path sliced the path with rsplit, which allocates a list and the unused tail; slice at rfind("/") instead. View prim paths are absolute, so rfind always hits at least the leading separator. - _initialize_fabric derived every child's parent path twice (once for the unique-parent list, again for the child->parent ordinal map); compute the list once and reuse it.
Address review feedback on isaac-sim#6805: the first-occurrence-wins guard only preserved list.index semantics for duplicate paths, but duplicates are invalid input and yield a wrong mapping under either occurrence choice, so keep the fastest form. Last occurrence now wins for a duplicate.
The nvbug 6535498 stall was invisible in Tracy and Nsight because the FrameView work happens in Python between Kit zones, and sampling profilers kept missing it (py-spy nonblocking drops samples in long C calls; nsys Python sampling is fragile behind launcher processes). Named zones make the getter, selection-refresh, opposite-space recompute, and one-time init phases show up explicitly on whichever backend the carb profiler targets: Tracy zones in tracy captures, NVTX ranges under Nsight Systems. carb.profiler.begin() returns immediately when no profiler is active, so the decorators cost nothing outside profiling sessions.
Description
Camera world-pose resolution stalled at high environment counts on the PhysX backend, badly enough to time out benchmarks. There are two independent bottlenecks, one per commit.
1.
FabricFrameViewresolved prim paths on the host, against the whole stageCameras read their poses from Fabric. To do that the frame view needs to know where each camera's data sits in Fabric memory, so it builds a lookup table from "camera number" to "Fabric slot".
Building that table was very slow. The view selected prims by requiring the Fabric world and local matrix attributes — but every xformable in the stage carries those, so the selection matched the entire stage (~1.1M prims at 8192 environments), not the view's handful of cameras. Finding its own prims in that list then meant building a Python dictionary over all of it, on the host:
That ran twice per rebuild (once for the children, once for their parents), and rebuilt whenever
PrepareForReuse()reported a bucket change — in practice on every environment reset.Two costs follow. The obvious one is that the work scales with the size of the whole scene. The less obvious one is that it creates a very large number of short-lived Python objects, stressing the garbage collector.
Fix: author a private per-view
uintindex attribute on each managed prim (and on each unique parent), holding that prim's view index. Every selection requires the matching index attribute, so selections resolve to exactly the view's prims. The view-to-Fabric slot mapping is then rebuilt by inverting that attribute in a single Warp kernel launch:That is
O(view)device work with no host-side path resolution and no Python objects created per frame.There is also no cache, so there is nothing to invalidate. The table is rebuilt from live Fabric data on every access, which means a bucket reorder can never leave a stale mapping behind. Selections are still checked with
GetCount(), which is exact here: the index attribute belongs to one view, so the count can only change if one of that view's prims actually disappeared — and then the view raises a clear error instead of silently reading the wrong prim.Attribute names embed a process-wide monotonic uid, so a dead view's leftover attributes can never satisfy a live view's selection. Parent reads get their own read-only selection, keeping the child RO/RW flip semantics from #5677 intact.
2.
SceneDataProvider.create_mappingdid a linear scan per itemFix: build the reverse dict once (first occurrence wins, matching
list.indexsemantics), then resolve each path inO(1).How the problem was found
The stall was invisible in Tracy. Comparing captures before and after the commit that introduced it, Kit's own per-frame work is unchanged —
App Updatetotals 65.1 s before and 62.3 s after, over ~120 frames. All the lost time sits in the gaps between frames, where the main thread is blocked in Python and Kit records nothing.A
py-spycapture found the cause:_compute_fabric_indices_foraccounted for 11.86% of samples and the garbage collector for a further 1.47%, reached throughcamera.reset()→get_world_poses().Fixes nvbug 6535498.
Type of change
Benchmarks
End-to-end symptom
Task
Isaac-Lift-KukaAllegro-Camera, 8192 environments,presets=physx,isaacsim_rtx_renderer,duo_camera. Commit69888c34e471(which introduced the problem) against its parente5f99d320338:GPU and CPU utilization both drop during the slow steps, which is what a stall looks like — the pipeline is waiting, not doing extra work.
Selection size — the actual fix
L40,
cuda:0, 1024-prim view (1024 children + 1024 parents). "Filler" is xformable prims not in the view but carrying Fabric matrices — i.e. the rest of a real scene.developselectiondevelopscales with the stage; this PR is pinned to the view.Slot-mapping rebuild (mean ms, the path hit on every reset)
developchilddevelopparentdevelopfull RO rebuilddevelopgrows linearly with stage size — 556 ms per rebuild at 100k filler prims. This PR is flat at ~0.14 ms, a ~2000× reduction, and unchanged from 0 to 100k filler prims. First access at 100k filler: 1824 ms → 383 ms.create_mapping(pure Python, reversed path order)developOutputs verified identical at every N. At the ~200k rigid bodies of an 8192-environment scene the old path takes minutes.
Tests
isaaclab_physx/test/sim/test_views_xform_prim_fabric.pyisaaclab/test/sim/test_views_xform_prim.py(USD contract)isaaclab_newton/test/sim/test_views_xform_prim_newton.pyisaaclab/test/sensors/test_camera.py(main consumer)isaaclab/test/utils/warp/test_proxy_array.pyNew:
test_selections_match_only_the_view_primsasserts each selection matches exactly the prims the view manages. Verified that it fails without the fix — with the selections unscoped it reportsmatched 8 prims, expected 4, because the child selections pick up the parents too.test_fabric_rebuild_after_topology_changewas updated to drive the new refresh paths (both child selections plus the parent selection) instead of the removed_rebuild_{ro,rw}_arrays.The 4 skips are environmental: empty device parameter sets, and Fabric hierarchy bindings that are unavailable in a headless experience.
Coverage gap:
test_physx_scene_data_backend.pyandtest_ovphysx_scene_data_backend.pyboth skip at collection on the machine used here, so thecreate_mappingchange is exercised only by the standalone benchmark above, not by the test suite. Worth a look in CI, where those backends are available.Screenshots
Not applicable — no visual change. The benchmarks above cover the behaviour change.
Notes for reviewers
Index attributes are never removed, and they accumulate.
_initialize_fabricauthors the index attribute but nothing drops it, so every view over the same prims leaves anotheruintbehind. Measured over 8 successive views on one stage (512 prims), attributes on a single prim went 1 → 8 and first-access rose monotonically from 120 ms to 173 ms; construction and rebuild times stayed flat. The uid scheme keeps this correct, but it is a per-stage leak that grows with view churn. Worth deciding whether the view should remove its attributes on teardown — there is currently no__del__/closehook.Relationship to #6554
#6554 found the same two slow paths. This PR takes its
SceneDataProviderfix unchanged (credited with aCo-authored-byline) and replaces its Fabric frame view fix.#6554 kept the whole-stage selection and the Python dictionary, and cached the result, reusing it while the number of selected prims stayed the same. Two problems with that. The cache key is unsafe: an equal prim count does not mean the prims or their order stayed the same, so after a bucket reorder or a same-count membership change the cached indices point at the wrong prims and cameras silently read another prim's transform. And it treats the symptom — the underlying operation is still proportional to the size of the whole scene, just performed less often.
Making the selection small removes the need for a cache at all, so both problems go away.
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