Skip to content

Caches find_joints and find_bodies on Articulation#5202

Open
hougantc-nvda wants to merge 3 commits intoisaac-sim:developfrom
hougantc-nvda:hougantc/cache-pick-place-indices
Open

Caches find_joints and find_bodies on Articulation#5202
hougantc-nvda wants to merge 3 commits intoisaac-sim:developfrom
hougantc-nvda:hougantc/cache-pick-place-indices

Conversation

@hougantc-nvda
Copy link
Copy Markdown
Contributor

@hougantc-nvda hougantc-nvda commented Apr 7, 2026

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

  • New feature (non-breaking change which adds functionality)

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

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.
@github-actions github-actions bot added the isaac-lab Related to Isaac Lab team label Apr 7, 2026
@hougantc-nvda hougantc-nvda requested a review from kellyguo11 April 7, 2026 22:30
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 7, 2026

Greptile Summary

This PR caches results of find_joints and find_bodies on both the PhysX and Newton Articulation implementations using per-instance @functools.cache closures created in a new _init_finder_caches() helper. The public methods return shallow copies of cached lists to prevent mutation of the cache. Supporting tests verify identity isolation and independence between repeated calls.

Confidence Score: 5/5

Safe 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.

Vulnerabilities

No security concerns identified.

Important Files Changed

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
Loading

Reviews (2): Last reviewed commit: "Use functools.cache for find_joints and ..." | Re-trigger Greptile

@hougantc-nvda hougantc-nvda requested review from kellyguo11 and rwiltz and removed request for kellyguo11 April 8, 2026 15:30
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot bot left a comment

Choose a reason for hiding this comment

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

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 str and Sequence[str] inputs
  • Both PhysX and Newton implementations are kept in sync

Minor observations (non-blocking):

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.
@hougantc-nvda
Copy link
Copy Markdown
Contributor Author

@greptileai

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 8, 2026

Tip:

Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.

Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.

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

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant