pref: FabricFrameView selection cache - #6893
Draft
pv-nvidia wants to merge 22 commits into
Draft
Conversation
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".
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.
Both _recompute_local_from_world_all and _recompute_world_from_local_all need the world and local child arrays, and reached them through _get_world_ifa and _get_local_ifa. Each accessor refreshes the active child selection independently, so every writer-scope exit ran PrepareForReuse, the count check and the slot-mapping kernel twice against the same selection. Add _get_child_ifas, which refreshes once and builds both indexed arrays from that refresh, and use it in both recomputes. Also trims the class docstring to the externally observable contract: the private selection names, kernel launches and error mechanics belong beside the helpers that implement them, and would go stale here. The tag-accumulation caveat moves in, since that one is a lifetime constraint callers need to know about.
The dict comprehension introduced earlier in this PR resolved a duplicate path to its last occurrence, where the list.index scan it replaced resolved to the first. That is a silent behaviour change on a public method for input the signature does not reject. It was taken on the assumption that the guard costs performance. It does not at the size that motivated the fix: over 200k paths the guarded loop measures 19.21 ms against 19.28 ms for the unguarded comprehension. The comprehension only wins below ~20k paths, by a fraction of a millisecond. Restore the guard, so the O(N^2) fix carries no semantic change.
Review feedback: several comments added while iterating on this PR explain rejected alternatives or restate mechanics the code already shows, and both changelog entries described the implementation rather than the user-visible outcome. - Kernel docstrings state their contract; the int32 index-array constraint is explained once, at the ArrayInt32_1d alias where it originates, instead of three times. - _parent_path documents its absolute-path precondition as an Args entry rather than narrating why it does not use rsplit. - The topology test no longer claims to cover topology recovery. It does not provoke a bucket change, so it is a smoke test of the refresh paths; the selection-scope test is the regression coverage. - Changelog entries state the outcome (stalls at high environment and rigid-body counts) and drop the algorithm description, which also removes an inaccurate O(N) claim about dictionary lookup.
Reverts the zone decorators added in 75b159f. A survey of the source tree found no other carb.profiler usage and no zone instrumentation of any kind, so these were a one-off style. They also made the module require Kit's paths at import time, where carb was previously only imported lazily. The zones remain useful for profiling sessions; re-apply 75b159f locally when needed.
The per-view index attributes FabricFrameView authors were never removed, so views recreated over the same prims on a long-lived stage accumulated attribute pairs, widening those prims' Fabric buckets and slowing later view initialization (measured: first access grew from 120 ms to 173 ms over 8 recreations at 512 prims). Add close(), which removes the view's tags and is safe to call more than once. BaseFrameView gains a no-op close() so callers can close any backend uniformly; Camera closes its view when invalidated. Views dropped without close() are cleaned up from __del__, following the shutdown-safe idiom used by the env classes: sys is bound as a default argument, nothing runs during interpreter finalization (Kit may be torn down, and Fabric dies with the process anyway), and a warning names the view so the missing close() call can be fixed. Removal is cheap (measured 4.7 us per prim) and proceeds past individual failed handles.
The missing-close() warning fired six times per suite run: the view_factory bundles were only torn down by the shared contract wrappers, and several tests build views directly and drop them. Register view.close as a pytest finalizer in the factory (idempotent, so tests that already close or tear down are unaffected) and close the directly-built views at the end of their tests. The garbage-collection test now builds its view inline: the fixture's finalizer is a bound method that would keep the view alive past the del it depends on. The one remaining warning per run comes from that test, which asserts the warning is emitted.
The lazily created views in prim_world_positions and SceneAsset were dropped without close(), so their Fabric index attributes were removed from __del__ with a warning naming the view. Trim the create_mapping comment to the mapping's contract.
Camera only closed its view on invalidation, so a dropped camera left the view to __del__ and logged the missing-close() warning.
The view-to-Fabric slot mapping was rebuilt on every accessor call, which kept the view immune to bucket reorders but made steady-state reads 3-4x slower than they need to be (0.20 ms vs 0.05-0.07 ms per call at 1024 prims). PrepareForReuse() already reports whether the selection's batch view was regenerated: False guarantees the slot layout is unchanged, so the indexed fabric arrays from the previous rebuild are still valid. Reuse them until it returns True (or the active selection flips between the RO and RW variants). The count check moves to the rebuild path, which is equivalent: the index tag is authored by this view alone, so the selection can only change through a topology change, and any topology change forces a rebuild. Set ISAACLAB_DISABLE_FABRIC_VIEW_CACHE=1 to rebuild on every access again when diagnosing suspected stale-mapping issues.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Follow-up to #6805. That PR rebuilds the view-to-Fabric slot mapping on every accessor call. That is always correct, but it does work that is usually unnecessary: in steady-state stepping nothing moves between Fabric buckets, so the mapping from the previous call is still valid.
This PR caches the indexed fabric arrays and reuses them until Fabric itself says they might be stale. The signal is the return value of
PrepareForReuse(), which #6805 already called but ignored:Falseguarantees the selection's batch view — and with it the slot layout — is unchanged, so the cached arrays are returned as-is.True(or a flip between the read-only and read-write child selections) triggers the same rebuild as before: one Warp kernel launch over the view's index attribute.Why this is not the cache that #6554 had to remove: that cache was keyed on the selection's prim count, and an equal count does not mean the prims or their order stayed the same — a bucket reorder preserves the count but shuffles slots, silently mapping cameras to the wrong prims. This cache is keyed on Fabric's own change tracking instead of a fingerprint we reconstruct: any event that could shuffle slots is a topology change, and every topology change makes
PrepareForReuse()returnTrue.Two details worth noting:
PrepareForReuse()is still called on every access, even on a cache hit. Its side effect (marking the RW selection's attributes dirty for downstream change tracking) is load-bearing for the renderer.GetCount()error check (a managed prim disappeared → clearRuntimeError) moves to the rebuild path. This detects the same failures at the same time: the index tag is authored by this view alone, so its selection can only change through a topology change, and any topology change forces a rebuild.Escape hatch
Set
ISAACLAB_DISABLE_FABRIC_VIEW_CACHE=1to rebuild on every access again (the exact #6805 behavior). If a pose ever looks stale, this gives a one-variable A/B test that separates "the cache key is wrong" from "the data is wrong": if the flag changes the result,PrepareForReuse()under-reported a topology change and that is a Fabric bug to escalate.Type of change
Benchmark
RTX A6000,
cuda:0, 1024-prim view, mean of 200 steady-state calls:get_world_posesget_local_posesThis removes the one regime where #6805 was slower than the code it replaced: per-operation cost on a static stage. End-to-end at high environment counts the accessor path was already negligible (~0.01% of step time), so no change is expected there.
Tests
isaaclab_physx/test/sim/test_views_xform_prim_fabric.py: 83 passed, 4 skipped (environmental: empty device parameter sets, headless Fabric hierarchy bindings).New tests:
test_child_arrays_cached_until_selection_flips— back-to-back accesses return the identical cached arrays; an RO↔RW flip forces a rebuild.test_cache_disabled_by_environment_variable— withISAACLAB_DISABLE_FABRIC_VIEW_CACHE=1every access rebuilds and reads stay correct.The existing
test_fabric_rebuild_after_topology_changenow exercises the rebuild path through the cached accessors.Screenshots
Not applicable — no visual 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