Caches find_joints and find_bodies on Articulation#5202
Caches find_joints and find_bodies on Articulation#5202hougantc-nvda wants to merge 3 commits intoisaac-sim:developfrom
Conversation
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.
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
source/isaaclab_physx/isaaclab_physx/assets/articulation/articulation.py
Outdated
Show resolved
Hide resolved
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):
source/isaaclab_physx/isaaclab_physx/assets/articulation/articulation.py
Show resolved
Hide resolved
source/isaaclab_physx/isaaclab_physx/assets/articulation/articulation.py
Show resolved
Hide resolved
source/isaaclab_physx/isaaclab_physx/assets/articulation/articulation.py
Outdated
Show resolved
Hide resolved
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.
|
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. |
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.
Cache the results using per-instance @functools.cache closures created in a new _init_finder_caches() helper. On cache hit the decorator returns the stored result directly; the public methods return shallow copies so callers cannot mutate the cached data. Applied to both the PhysX and Newton Articulation implementations.
Description
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