Caches resolve_matching_names on AssetBase for all finder methods#5202
Caches resolve_matching_names on AssetBase for all finder methods#5202hougantc-nvda wants to merge 17 commits intoisaac-sim:developfrom
Conversation
Greptile SummaryThis PR caches results of Confidence Score: 5/5Safe to merge — the caching logic is correct, tests verify mutation isolation, and no P0/P1 issues were found. All findings are P2 or lower. The per-instance closure pattern is a valid approach for caching results that depend on immutable instance state. Shallow copies on return correctly prevent cache corruption. Tests explicitly validate that repeated calls return independent list objects. Changelog entries and version bumps are consistent. No files require special attention.
|
| Filename | Overview |
|---|---|
| source/isaaclab_physx/isaaclab_physx/assets/articulation/articulation.py | Adds _init_finder_caches() to create per-instance @cache closures; find_bodies and find_joints delegate to them and return shallow copies. Correct design given immutable body/joint names post-construction. |
| source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py | Mirrors the PhysX changes: same _init_finder_caches() helper and updated find_bodies/find_joints wrappers. Symmetrical and consistent implementation. |
| source/isaaclab/test/assets/test_articulation_iface.py | Adds test_find_* tests covering return types, single-match, all-match, and mutation isolation (verifying returned lists are independent copies). Tests call _init_finder_caches() directly in the mock setup path. |
| source/isaaclab_physx/docs/CHANGELOG.rst | New 0.5.14 version heading added; version bumped to match extension.toml. RST formatting and Sphinx cross-references look correct. |
| source/isaaclab_newton/docs/CHANGELOG.rst | New 0.5.11 version heading added; version bumped to match extension.toml. RST formatting and Sphinx cross-references look correct. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Articulation
participant Cache as @cache closure
participant Resolver as resolve_matching_names
Note over Articulation: __init__() calls _init_finder_caches()
Articulation->>Cache: create _find_bodies_cached (per-instance)
Articulation->>Cache: create _find_joints_cached (per-instance)
Caller->>Articulation: find_bodies(name_keys)
Articulation->>Articulation: normalize keys to tuple
Articulation->>Cache: _find_bodies_cached(keys, preserve_order)
alt Cache miss (first call)
Cache->>Resolver: resolve_matching_names(keys, self.body_names, preserve_order)
Resolver-->>Cache: (list[int], list[str])
Cache-->>Articulation: cached result
else Cache hit
Cache-->>Articulation: stored result (no resolver call)
end
Articulation->>Articulation: list(result[0]), list(result[1]) shallow copy
Articulation-->>Caller: (list[int], list[str]) independent copy
Reviews (2): Last reviewed commit: "Use functools.cache for find_joints and ..." | Re-trigger Greptile
There was a problem hiding this comment.
PR Review: Caches find_joints and find_bodies on Articulation
Nice optimization — the Nsight profiling data makes a clear case for this, and the implementation is clean. Caching deterministic regex lookups that always return the same result is a solid win for hot-loop performance.
What looks good:
- Shallow-copy defense (
list(result[0]), list(result[1])) correctly prevents callers from mutating cached data - Tests explicitly verify identity independence (
is not) and mutation isolation — exactly the right tests for this change - Cache key construction properly normalizes both
strandSequence[str]inputs - Both PhysX and Newton implementations are kept in sync
Minor observations (non-blocking):
|
Tip: Greploops — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
AntoineRichard
left a comment
There was a problem hiding this comment.
Thanks for the optimization, I think it's important some of these results get cached to avoid overhead. However, right now, this PR misses the rest of the assets (RigidObjectCollection, RigidObject) as well as some other properties in the Articulation (Tendons, SpatialTendons). Could you extend the pattern to these. Also I would consider relying on an LRU cache rather than our own custom caching. I'm not completely against the custom caching, but I'd be curious to know why a custom cache was favored over the LRU cache.
261180e to
2b7c64d
Compare
AntoineRichard
left a comment
There was a problem hiding this comment.
Made the code simpler following @camevor comment.
|
I ran this branch through profiler and compared it against develop NVTX range Articulation.find_joints The optimization is working as intended. If no other comments, we can merge. |
Articulation.find_joints and find_bodies delegate to resolve_matching_names, which runs a regex double-loop on every call. Nsight profiling of the pick-place task shows this costs ~881μs per call (~1.83ms / 1.5% of step time) for work that always returns the same result, since joint and body names are fixed after construction. Add per-instance dict caches keyed on the (normalized) call arguments. On cache hit the methods return shallow copies of the stored lists so callers cannot mutate the cached data. Applied to both the PhysX and Newton Articulation implementations.
Verify that repeated calls return equal but independent lists, and that mutating a returned list does not corrupt subsequent results.
Replace hand-rolled dict caches with per-instance @functools.cache closures created in a new _init_finder_caches() helper. This keeps the cache per-instance (cleaned up on GC) while letting the decorator handle key hashing and storage. Extract _init_finder_caches() as a separate method so test fixtures that bypass __init__ via object.__new__() can also initialize the caches, fixing all TestArticulationFinders tests on physx/newton backends.
Lift the per-instance @functools.cache closures out of the Newton and PhysX articulation classes into AssetBase so every asset (Articulation, RigidObject) benefits from cached name resolution without duplicating the mechanism. - Add _resolve_matching_names_cached and _resolve_matching_names_values_cached wrapper methods that handle argument conversion and return independent list copies. - Replace direct resolve_matching_names / resolve_matching_names_values calls in Newton/PhysX articulation and rigid-object finders. - Add cache lifecycle tests: GC cleanup, instance isolation, error propagation, mutation safety, preserve_order, multi-pattern input, and cached-vs-uncached equivalence. - Remove redundant repeated-calls-are-independent tests now covered by the cache mutation test. - Initialize caches in rigid-object test factories that bypass __init__.
Spec for issue isaac-sim#5316: implementing RigidObject + RigidObjectData for the OVPhysX backend. Mirrors PhysX structure using ovphysx TensorBindings. Validation environment: Isaac-Repose-Cube-Allegro-Direct-v0.
Design specs for the full OVPhysX integration: - RigidObjectCollection (isaac-sim#5317) - ContactSensor (new issue needed) - IMU sensor (isaac-sim#5318) - PVA sensor (isaac-sim#5319) - FrameTransformer sensor (isaac-sim#5320) - DeformableObject (new issue needed) - SceneDataProvider (new issue needed) - Rough terrain locomotion (isaac-sim#5321) - Manipulation support (isaac-sim#5322) - Remove start script (isaac-sim#5323) - USD integration improvement (isaac-sim#5324) - Test infrastructure (new issue needed)
Three AssetBase subclasses intentionally skip super().__init__() to avoid unwanted spawning logic, which means the new _init_resolve_matching_names_caches() call added to AssetBase.__init__ never executes. Add explicit calls in each.
Replace the static _find_names method with _resolve_matching_names_cached inherited from AssetBase. This gives OVPhysX the same per-instance caching that PhysX and Newton already have for find_bodies, find_joints, find_fixed_tendons, and find_spatial_tendons. Also aligns find_joints joint_subset type from list[int] to list[str] to match the base class contract.
Replace per-instance caching on AssetBase with a module-level @functools.cache on the private _resolve_matching_names_impl helper in string.py. This gives the same performance benefit (cached regex matching for find_joints / find_bodies) with no per-instance infrastructure, no extra call-site changes, and ~160 fewer lines. resolve_matching_names_values is left uncached because all callers use it during init only, never in the step loop.
- Create new isaaclab 4.6.8 version heading instead of adding to existing 4.6.7 (immutable). Bump extension.toml to match. - Revert newton changelog section reorder (existing versions are immutable). - Clarify _resolve_matching_names_impl docstring about tuple protection. - Soften resolve_matching_names_values note to say "current callers" instead of "all callers". - Document fnmatch deprecation and empty-tendon ValueError in ovphysx changelog.
The parameter type changed from list[int] to list[str], which is a behavioral change, not just a type hint fix. Add migration guidance.
The module-level functools.cache on _resolve_matching_names_impl grows unbounded across scene rebuilds in long-lived processes. Add a public clear_resolve_matching_names_cache() helper and call it from SimulationContext.clear_instance() so cached entries from destroyed assets are freed when the scene is torn down.
f965bcf to
2ef9c49
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR adds module-level @functools.cache to resolve_matching_names, eliminating ~881μs/call of redundant regex matching for find_joints/find_bodies in the hot loop. The implementation is clean: cached results are immutable tuples, and the public wrapper returns fresh list copies to prevent caller mutation. The cache is properly cleared via SimulationContext.clear_instance().
Architecture Impact
Cross-module impact is well-contained:
resolve_matching_namesis used by all backend articulations (PhysX, Newton, OVPhysX) and rigid objects — they all benefit automatically- The OVPhysX articulation finder methods now delegate to the standard
resolve_matching_namesinstead of a private_find_namesusing fnmatch, unifying behavior across backends - Cache clearing is hooked into
SimulationContext.clear_instance(), so long-lived processes that rebuild scenes do not accumulate stale entries
Implementation Verdict
Ship it — This is a solid optimization with clean implementation. The PR has been well-iterated based on reviewer feedback (moved from per-instance caches to module-level, addressed unbounded growth concern). No blocking issues found.
Test Coverage
✅ Good coverage:
test_clear_resolve_matching_names_cache: Verifies cache clears correctly and results remain correct after clearingtest_mutating_result_does_not_corrupt_cache: Confirms the shallow-copy defense works — callers cannot corrupt cached datatest_unmatched_regex_raises: Verifies ValueError propagation through the cache layertest_find_with_multiple_patternsandtest_find_with_preserve_order: Confirms correct behavior with list patterns and ordering
The tests cover the key invariants: cache correctness, mutation safety, error propagation, and clearing.
CI Status
Installation Tests,Build Base Docker Image,Build cuRobo Docker Image,Build Latest Docsall failed with Docker buildkit errors (open /var/lib/docker/buildkit/executor/resolv.conf.tmp: no such file or directory)pre-commit✅ passed- These are transient runner issues, not code problems
Findings
🔵 Observation: OVPhysX find_fixed_tendons/find_spatial_tendons now raise on empty lists
The OVPhysX changelog correctly documents this, but worth noting: the previous implementation returned ([], []) when tendon_subsets was empty, while the new implementation (via resolve_matching_names) will raise ValueError because the regex won't match anything. This aligns with PhysX backend behavior, so it's a good consistency fix.
🔵 Observation: The joint_subset type change in OVPhysX is a breaking change
Changed from list[int] (indices) to list[str] (names). The changelog documents this correctly with migration guidance. This aligns with the BaseArticulation interface contract, so existing OVPhysX users passing indices will need to update their code.
Overall: Clean, well-tested optimization with proper cache lifecycle management. The PR has addressed all prior review feedback. Ready to merge once CI infrastructure issues clear.
Description
Articulation.find_jointsandfind_bodiesdelegate toresolve_matching_names, which runs a Python regex double-loop on every call. Nsight profiling of the pick-place task showed this costs ~881μs per call (~1.83 ms / 1.5% of step time) for work that always returns the same result — joint and body names are fixed after construction.This PR adds a module-level
@functools.cacheon a new private_resolve_matching_names_implhelper. Hashable tuples are used for the cache key; cached results are immutable tuples that the public wrapper copies into fresh lists per caller, so callers cannot mutate shared state.resolve_matching_names_valuesis left uncached because its current callers only use it during init, never in the step loop.A
clear_resolve_matching_names_cache()helper is called fromSimulationContext.clear_instance()so cached entries from destroyed assets do not accumulate across scene rebuilds in long-lived processes.Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there