Skip to content

Fix O(N)/O(N^2) camera-pose resolution stall under PhysX + Newton ren… - #6554

Closed
yts-nv wants to merge 6 commits into
isaac-sim:developfrom
yts-nv:fix/physx-newton-camera-pose-scaling
Closed

Fix O(N)/O(N^2) camera-pose resolution stall under PhysX + Newton ren…#6554
yts-nv wants to merge 6 commits into
isaac-sim:developfrom
yts-nv:fix/physx-newton-camera-pose-scaling

Conversation

@yts-nv

@yts-nv yts-nv commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Description

At high environment counts (e.g. Isaac-Lift-KukaAllegro-Camera at 8192 envs) with
presets=physx,newton_renderer, the PhysX-backed camera-sensor pose update stalls for
minutes per training iteration, causing benchmarks to time out with no results.

Root cause: two per-prim Python hotspots on the PhysX -> Newton-renderer camera path,
both scaling with the whole-scene prim/body count rather than the handful of managed cameras:

  1. FabricFrameView._compute_fabric_indices_for rebuilt a {str(Sdf.Path): index} map over
    every prim in the Fabric selection (~1.14M prims for an 8192-env scene) on every env
    reset
    , only to resolve the managed camera prims.
    -> Cache the map per selection; rebuild only when the selection size changes.

  2. SceneDataProvider.create_mapping used an O(N^2) list.index() lookup per input transform
    (~200k rigid bodies).
    -> Replace with an O(N) dict lookup.

Results

Measured on Isaac-Lift-KukaAllegro-Camera, presets=physx,newton_renderer,single_camera,rgb64,
num_envs=8192 (RTX PRO 6000 Blackwell):

before after
per-iteration ~220 s ~53 s
outcome timeout, no results completes, KPIs written

Training rewards are unchanged (verified at num_envs=256; per-iteration mean reward identical).

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Performance improvement

Checklist

  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@github-actions github-actions Bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Jul 16, 2026
@greptile-apps

greptile-apps Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes two separate per-prim Python hotspots on the PhysX → Newton-renderer camera path that caused per-iteration stalls of ~220 s at 8192 environments, reducing it to ~53 s.

  • FabricFrameView._compute_fabric_indices_for: caches the str(Sdf.Path) → fabric-index dict per selection object (keyed by id(selection) + size), avoiding a full rebuild over all ~1.1 M prims in the scene on every environment reset.
  • SceneDataProvider.create_mapping: replaces an O(N²) list.index() inner loop (~200k rigid bodies) with a single O(N) dict build followed by O(1) lookups per input path, preserving first-occurrence and missing-path (-1) semantics exactly.

Confidence Score: 4/5

Safe to merge for the described production use case; the one concern is a code-pattern fragility in the caching strategy that is unlikely to trigger in the steady-state training loop.

Both hotfixes are correct and well-scoped. The scene_data_provider.py change is a clean algorithmic substitution with identical semantics. The fabric_frame_view.py cache uses id(selection) as its key, which can theoretically serve a stale dict if a selection object is GC'd and a new one lands at the same address with the same prim count — a silent correctness hazard. In the current training loop, selections are long-lived view attributes so the risk is low, but it is a structural fragility worth noting.

The cache implementation in fabric_frame_view.py around the id(selection) keying deserves a second look; scene_data_provider.py is straightforward and needs no special attention.

Important Files Changed

Filename Overview
source/isaaclab_physx/isaaclab_physx/sim/views/fabric_frame_view.py Caches the expensive path→Fabric-index dict keyed by id(selection); addresses the O(N) str conversion over all 1.1M scene prims on every reset, with one minor robustness concern around Python id-reuse.
source/isaaclab/isaaclab/scene_data/scene_data_provider.py Replaces O(N²) list.index() per input path with a single O(N) dict build + O(1) lookups; semantics match the original (first-occurrence wins, missing paths → -1, None output slots skipped).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Camera pose update triggered] --> B[FabricFrameView._compute_fabric_indices_for]
    B --> C{Cache hit for selection?}
    C -- Yes, same id + same size --> D[Reuse cached path-to-idx dict]
    C -- No --> E[Build str path dict over all selection prims]
    E --> F[Store entry in _fab_pathmap_cache]
    F --> D
    D --> G[Lookup managed camera paths]
    G --> H[Return wp.array of fabric indices]
    I[SceneDataProvider.create_mapping] --> J[Build path-to-out_idx dict over output paths O-N]
    J --> K[Resolve each input_path via dict.get O-1 each]
    K --> L{mapping == identity?}
    L -- Yes --> M[Return None]
    L -- No --> N[Return wp.int32 remapping array]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[Camera pose update triggered] --> B[FabricFrameView._compute_fabric_indices_for]
    B --> C{Cache hit for selection?}
    C -- Yes, same id + same size --> D[Reuse cached path-to-idx dict]
    C -- No --> E[Build str path dict over all selection prims]
    E --> F[Store entry in _fab_pathmap_cache]
    F --> D
    D --> G[Lookup managed camera paths]
    G --> H[Return wp.array of fabric indices]
    I[SceneDataProvider.create_mapping] --> J[Build path-to-out_idx dict over output paths O-N]
    J --> K[Resolve each input_path via dict.get O-1 each]
    K --> L{mapping == identity?}
    L -- Yes --> M[Return None]
    L -- No --> N[Return wp.int32 remapping array]
Loading

Reviews (1): Last reviewed commit: "Fix O(N)/O(N^2) camera-pose resolution s..." | Re-trigger Greptile

Comment on lines +740 to +745
entry = cache.get(id(selection))
if entry is not None and entry[0] == n:
path_to_idx = entry[1]
else:
path_to_idx = {str(p): i for i, p in enumerate(all_paths)}
cache[id(selection)] = (n, path_to_idx)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 id() reuse can silently serve a stale index map

Python reuses memory addresses: if a selection object is garbage-collected and a new object is later allocated at the same address, id(new_selection) == id(old_selection) and the cache would return the old path→index dict. The size check (entry[0] == n) only guards against a different-sized selection landing at that address; a same-sized new selection would pass it and get wrong fabric indices — silently, with no error.

In the current steady-state training loop the selection objects are long-lived view attributes, so this is unlikely to fire. A structurally sound alternative is weakref.WeakKeyDictionary (requires the selection type to support weak-referencing), which auto-evicts entries on GC and uses actual object identity rather than potentially reused addresses. If the selection is not weakly-referenceable, caching the map as an instance attribute keyed by selection itself (if hashable) avoids the id-reuse hazard too.

@yts-nv
yts-nv force-pushed the fix/physx-newton-camera-pose-scaling branch 2 times, most recently from 3417f85 to ab9e44b Compare July 16, 2026 05:01
…derer

At high environment counts (e.g. Isaac-Lift-KukaAllegro-Camera at 8192 envs),
the PhysX-backed camera sensor pose update stalls for minutes per training
iteration, causing benchmarks to time out with no results.

Two per-prim Python hotspots on the PhysX -> Newton-renderer camera path:

1. FabricFrameView._compute_fabric_indices_for rebuilt a
   {str(Sdf.Path): index} map over EVERY prim in the Fabric selection
   (~1.14M prims for an 8192-env scene) on every env reset, only to resolve
   the managed camera prims. Cache the map per selection and rebuild it only
   when the selection size changes.

2. SceneDataProvider.create_mapping used an O(N^2) list.index() lookup per
   input transform (~200k rigid bodies). Replace it with an O(N) dict lookup.

Measured on Isaac-Lift-KukaAllegro-Camera,
presets=physx,newton_renderer,single_camera,rgb64, num_envs=8192
(RTX PRO 6000 Blackwell): per-iteration time drops from ~220 s to ~53 s and
the run completes and writes benchmark KPIs instead of timing out. Training
rewards are unchanged (verified at num_envs=256).
@yts-nv
yts-nv force-pushed the fix/physx-newton-camera-pose-scaling branch from ab9e44b to 051dec8 Compare July 16, 2026 05:18
@yts-nv
yts-nv requested a review from a team July 22, 2026 02:13
@yts-nv
yts-nv requested a review from StafaH as a code owner July 28, 2026 08:38
@pv-nvidia pv-nvidia self-assigned this Jul 30, 2026
See :meth:`_rebuild_ro_arrays`: the resolved index arrays are recomputed
only when the selection size changes; the wrappers are always rebuilt.
"""
n = len(self._sel_rw.GetPaths())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caching on just the length feels fragile. But better caching like computing a hash might make this slow again...

changes; the indexed-fabric-array wrappers below are always rebuilt since
the underlying buckets may have moved.
"""
n = len(self._sel_ro.GetPaths())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caching on just the length feels fragile. But better caching like computing a hash might make this slow again...

self._ro_child_idx_host = self._compute_fabric_indices(self._sel_ro).numpy()
self._ro_parent_idx_host = self._compute_parent_fabric_indices(self._sel_ro).numpy()
self._ro_cached_n = n
# Fresh device arrays each rebuild (do not alias one cached wp.array across

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment

if getattr(self, "_ro_cached_n", None) != n:
# Cache only the tiny resolved indices on the host (a few KB), NOT the
# giant per-selection path->index dict.
self._ro_child_idx_host = self._compute_fabric_indices(self._sel_ro).numpy()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why .numpy() here?

"""
n = len(self._sel_rw.GetPaths())
if getattr(self, "_rw_cached_n", None) != n:
self._rw_child_idx_host = self._compute_fabric_indices(self._sel_rw).numpy()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why numpy() here?

self._rw_child_idx_host = self._compute_fabric_indices(self._sel_rw).numpy()
self._rw_parent_idx_host = self._compute_parent_fabric_indices(self._sel_rw).numpy()
self._rw_cached_n = n
self._rw_fabric_indices = wp.array(self._rw_child_idx_host, dtype=wp.int32, device=self._device)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wny do we create new indices here? Can't we use the cached ones directly?

# the churning per-reset wrappers).
self._ro_fabric_indices = wp.array(self._ro_child_idx_host, dtype=wp.int32, device=self._device)
self._ro_parent_fabric_indices = wp.array(self._ro_parent_idx_host, dtype=wp.int32, device=self._device)
self._world_ifa_ro = self._build_indexed_array(self._sel_ro, self._WORLD_MATRIX_NAME, self._ro_fabric_indices)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wny do we create new indices here? Can't we use the cached ones directly?

pv-nvidia added a commit to pv-nvidia/IsaacLab that referenced this pull request Jul 30, 2026
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>
pv-nvidia added a commit to pv-nvidia/IsaacLab that referenced this pull request Jul 30, 2026
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.
pv-nvidia added a commit to pv-nvidia/IsaacLab that referenced this pull request Jul 30, 2026
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>
pv-nvidia added a commit to pv-nvidia/IsaacLab that referenced this pull request Jul 30, 2026
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.
pv-nvidia added a commit to pv-nvidia/IsaacLab that referenced this pull request Jul 31, 2026
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>
pv-nvidia added a commit to pv-nvidia/IsaacLab that referenced this pull request Jul 31, 2026
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.
pv-nvidia added a commit to pv-nvidia/IsaacLab that referenced this pull request Aug 1, 2026
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>
pv-nvidia added a commit to pv-nvidia/IsaacLab that referenced this pull request Aug 1, 2026
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.
pv-nvidia added a commit to pv-nvidia/IsaacLab that referenced this pull request Aug 2, 2026
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>
pv-nvidia added a commit to pv-nvidia/IsaacLab that referenced this pull request Aug 2, 2026
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.
@AntoineRichard

AntoineRichard commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Closing this PR in favor of #6805 to avoid accidental merging. If you feel different feel free to re-open it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants