Skip to content

PERF: G1Flat training throughput — contact sensor pooled reset + event sync elimination - #685

Open
kioxli wants to merge 4 commits into
mainfrom
perf/g1flat-throughput
Open

kioxli wants to merge 4 commits into
mainfrom
perf/g1flat-throughput

Conversation

@kioxli

@kioxli kioxli commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • contact_history: pooled allocations by dtype/shape family (10 separate tensors → 6 pools); reset() fused from 10 CUDA writes → 1 _foreach_zero_ (full reset) / 6 broadcast writes (subset reset)
  • event_manager: interval_step == 1 events take an env_ids=None fast path, eliminating a per-control-step GPU nonzero + host len() device synchronization

Why

G1 × 4096 NoRender training is bound by the host-side Python orchestration (~600k calls/control step, GPU busy <25%). The contact sensor reset chain (36 advanced-indexing writes per episode reset, ×26 resets/iteration early in training) and the per-step event scheduling sync were the two largest measurable contributors.

Benchmark

G1 × 4096, NoRender + newton physics, RTX 5090 D, 2 warmup + 10 iterations, passed, compiles=0:

Config train_e2e_sps
main (7ae9a5012) 80,568
+ this PR 99,398 (+23%)

GPU peak 9.1 GiB / 24.5 GiB.

@greptile-apps

greptile-apps Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

The PR needs the explicit repository requirements for the new test file and cache-reuse coverage satisfied before merging.

Fix All in CodexFindings

  1. P2 New test lacks required conventions ▶
  2. P2 Cache reuse remains untested ▶
Fix with agent prompt
### Issue 1
tests/gym/envs/managers/test_event_manager_interval_fastpath.py:1
This new file omits the required DexForce 2021–2026 Apache 2.0 copyright header and `from __future__ import annotations`. The multiline `self.calls.append` at lines 14–16 also needs the repository-required Black formatting. These requirements must be satisfied before merging.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

### Issue 2
tests/gym/envs/managers/test_event_manager_interval_fastpath.py:43
The test calls `apply()` only once, so it would pass if the interval-one path went back to allocating a new ID tensor on every control step. The repository requires focused tests that prove new production behavior. Assert that successive calls reuse the cached tensor to protect this throughput change before merging.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

The PR pools contact-history storage to reduce reset writes and replaces interval-one event ID allocation with a cached all-row tensor.

  • Adds a full contact-history reset test and an interval-one ID test.
  • The new test file needs repository-required formatting and metadata, and the cache-reuse behavior still needs a focused assertion.

Reviews (5) · Last reviewed commit: "FIX: address maintainer review — focused..."

Comment thread embodichain/lab/gym/envs/managers/event_manager.py Outdated
Comment thread embodichain/lab/sim/sensors/contact_history.py Outdated
Comment thread embodichain/lab/sim/sensors/contact_history.py Outdated
kioxli pushed a commit that referenced this pull request Sep 24, 2026
- event_manager: interval_step==1 fast path passes the explicit all-row
  ID tensor (torch.arange) instead of None — functors calling
  len(env_ids) (e.g. randomize_rigid_object_mass) keep working
- contact_history: black formatting on the pooled allocation
- tests: focused coverage for the env_ids=None fused reset path
Comment thread embodichain/lab/gym/envs/managers/event_manager.py Outdated
@yuecideng
yuecideng requested a review from acrlw September 24, 2026 08:27
G1×4096 NoRender 训练实测(RTX 5090 D):SPS 80.6k → 95.8k(+19%),passed。

- contact_history: 按 dtype/shape 家族池化分配(10 独立张量 → 6 池),
  重置从 10 次 CUDA 写融合为 foreach 1 次(全量)/ 6 次广播写(子集)
- event_manager: interval_step==1 的事件走 env_ids=None 快路径,
  消除每 control step 一次 GPU nonzero + 宿主 len() 设备同步

(contact_sensor 重置图与 reset_masked 为 graph 化预留,暂不入库)
- event_manager: interval_step==1 fast path passes the explicit all-row
  ID tensor (torch.arange) instead of None — functors calling
  len(env_ids) (e.g. randomize_rigid_object_mass) keep working
- contact_history: black formatting on the pooled allocation
- tests: focused coverage for the env_ids=None fused reset path
@kioxli
kioxli force-pushed the perf/g1flat-throughput branch from ef3628a to a95a655 Compare September 24, 2026 08:40
@yuecideng
yuecideng self-requested a review September 24, 2026 09:37
…ll-row IDs

- tests: focused EventManager test with a spy functor asserting env_ids is
  a torch.long tensor on env.device and equals torch.arange(env.num_envs)
  (regression guard for the interval_step==1 dispatch contract; functors
  calling len(env_ids) require explicit IDs)
- event_manager: cache the manager-owned all-row ID tensor during
  preparation and treat it as read-only (~0.03 µs reuse vs ~5 µs fresh
  torch.arange per call, per the maintainer's bounded probe)

Both findings confirmed on a95a655 by yuecideng; the P3 caching follows
the maintainer's suggested design.
@@ -0,0 +1,50 @@
"""interval_step==1 快路径:functor 收到显式全量 ID 张量(非 None)。"""

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 New test lacks required conventions

This new file omits the required DexForce 2021–2026 Apache 2.0 copyright header and from __future__ import annotations. The multiline self.calls.append at lines 14–16 also needs the repository-required Black formatting. These requirements must be satisfied before merging.

Context Used: CLAUDE.md (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/gym/envs/managers/test_event_manager_interval_fastpath.py
Line: 1

Comment:
**New test lacks required conventions**

This new file omits the required DexForce 2021–2026 Apache 2.0 copyright header and `from __future__ import annotations`. The multiline `self.calls.append` at lines 14–16 also needs the repository-required Black formatting. These requirements must be satisfied before merging.

**Context Used:** CLAUDE.md ([source](https://github.com/dexforce/embodichain/blob/main/CLAUDE.md))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Codex Fix in Claude Code

lambda mode, name, cfg, env, ids: received.append(ids),
)

manager.apply(mode="interval")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Cache reuse remains untested

The test calls apply() only once, so it would pass if the interval-one path went back to allocating a new ID tensor on every control step. The repository requires focused tests that prove new production behavior. Assert that successive calls reuse the cached tensor to protect this throughput change before merging.

Context Used: CLAUDE.md (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/gym/envs/managers/test_event_manager_interval_fastpath.py
Line: 43

Comment:
**Cache reuse remains untested**

The test calls `apply()` only once, so it would pass if the interval-one path went back to allocating a new ID tensor on every control step. The repository requires focused tests that prove new production behavior. Assert that successive calls reuse the cached tensor to protect this throughput change before merging.

**Context Used:** CLAUDE.md ([source](https://github.com/dexforce/embodichain/blob/main/CLAUDE.md))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Codex Fix in Claude Code

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants