Skip to content

perf(fastsac): async collector push 的拷贝冗余与同步阻塞 - #63

Merged
wlgys8 merged 1 commit into
mainfrom
perf/fastsac-async-push-61
Sep 21, 2026
Merged

wlgys8 merged 1 commit into
mainfrom
perf/fastsac-async-push-61

Conversation

@wlgys8

@wlgys8 wlgys8 commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Closes #61, Closes #62

基于 origin/main 的两个 perf commit(从 perf/fastsac-async-push cherry-pick 而来):

#61 — async collector push 的拷贝冗余与同步阻塞

  • CPU-env 路径上 transition-ring push 的拷贝开销减半(shm.py ring push 优化)
  • GPU-env 的异步 D2H staging 路径当前未接线(env 始终建在 CPU),留待 GPU-env 支持时再加

#62 — async learner 抢 CPU 核拖慢 collector

  • learner / collector 进程级 CPU 亲和性隔离,各自独立配置
  • core spec 语法:"0:5,7" 表示核 0–5 加 7;越界核自动丢弃;null 关闭该侧 pinning
  • 同时用 torch.set_num_threads 把线程数限制在各自核片内

Benchmark(g1-sonic 4096 envs):env_step 55 -> ~47ms,稳定吞吐 ~59k -> ~65k env-steps/s(对应 #62 配比表中 learner 2 核的最优档)。

cherry-pick 冲突解决:collector.py 中 main 侧使用构造参数 action_scale/action_bias,按分支侧改为 self.actor.action_scale(main 的 Actor 注册同名 buffer,语义等价)。

@wlgys8
wlgys8 force-pushed the perf/fastsac-async-push-61 branch 6 times, most recently from 7c14df9 to 152f309 Compare September 20, 2026 15:48
@wlgys8

wlgys8 commented Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

g1-wbt-dance 吞吐实测(RTX 5090 / 32 核,2048 envs,各 300 iters)

配置 稳态吞吐 (env-steps/s) 相对 main env_step (ms) push (ms)
main(基线) ~76k 23.4–24.5 0.53–0.61
PR,无 pinning ~78k +2.4% 22.9–23.6 0.33(−44%
PR + pin(learner 0:1 / collector 2:31 ~85k +11% 20.9–21.6 0.38

分解:

两项合计 +11%(76k -> 85k env-steps/s)。

注:300 iters 短程数据,iter200/iter300 采样点间波动约 ±1.5%;合并前可用 1000+ iters 复核稳态。

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The new CPU affinity implementation can crash on platforms or environments without os.sched_getaffinity/sched_setaffinity, and the ring now requires ring_capacity >= 2 to avoid deadlock but does not currently enforce it.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 2 Medium severity

Open (3)
What changed in this PR

This PR ports two performance-focused changes onto origin/main for the FastSAC async (two-process) trainer: reducing shared-memory ring copy/footprint by eliminating redundant next_obs storage, and adding optional per-process CPU affinity + thread capping to prevent the learner from stealing CPU resources from the collector.

Changes:

  • Optimize SharedTransitionRing to store 6 tensors per slot (derive next_obs/next_critic_obs from the successor slot) and update learner/collector handoff accordingly.
  • Add configurable CPU core pinning for async learner and collector processes, and cap Torch intra-op threads per process.
  • Update base config + tests to include the new async options and document the new knobs.
File Description
motrix_rl/​tests/​test_rl_sim_backend.py Extends async test config to include the new CPU pinning options.
motrix_rl/​src/​motrix_rl/​fastsac/​config.py Adds learner_cpu_cores / collector_cpu_cores to async options config.
motrix_rl/​src/​motrix_rl/​fastsac/​async_impl/​worker.py Implements CPU core spec parsing and applies pinning + torch.set_num_threads in worker processes.
motrix_rl/​src/​motrix_rl/​fastsac/​async_impl/​shm.py Removes next_obs storage from the transition ring and adds successor-slot accessors.
motrix_rl/​src/​motrix_rl/​fastsac/​async_impl/​learner.py Updates ring draining to wait for successor slot and derive next_obs/next_critic_obs.
motrix_rl/​src/​motrix_rl/​fastsac/​async_impl/​collector.py Updates ring push to match 6-field slots and uses actor buffers for action scaling.
configs/​algo_base/​motrix.fastsac.yaml Documents and exposes the new CPU affinity configuration knobs.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread motrix_rl/src/motrix_rl/fastsac/async_impl/shm.py
Comment thread motrix_rl/src/motrix_rl/fastsac/async_impl/worker.py Outdated
Comment thread motrix_rl/src/motrix_rl/fastsac/async_impl/worker.py

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

CPU 核 spec 解析目前可能因超大 range 枚举导致 worker 挂死/卡死,需要先补上范围校验与避免枚举巨大区间的实现修正。

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity · 1 Low severity

Open (2)
Resolved since last review (3)

Comment thread motrix_rl/src/motrix_rl/fastsac/async_impl/worker.py
Comment thread motrix_rl/src/motrix_rl/fastsac/async_impl/collector.py

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

torch.set_num_threads is skipped when affinity pinning fails (reducing the reliability of the intended CPU isolation), and the collector docstring currently contradicts the new “final batch may remain unconsumed” behavior.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
Resolved since last review (2)

Comment thread motrix_rl/src/motrix_rl/fastsac/async_impl/worker.py

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The new ring_capacity >= 2 contract breaks an existing in-repo benchmark utility that still constructs SharedTransitionRing(1, ...), and that call site should be updated (or the API explicitly accommodate that use).

Review effort: Lite
Findings: None

Resolved since last review (1)

…king

Closes #61, Closes #62

- halve transition-ring push cost on the CPU-env path (shm.py ring push
  copy optimization); the GPU-env async D2H staging path is not wired up
  yet and will be added together with GPU-env support (#61)
- per-side CPU affinity pinning for the async learner/collector processes,
  configured as core specs like "0:5,7" via learner_cpu_cores /
  collector_cpu_cores (#62)

Benchmark (g1-sonic 4096 envs): env_step 55 -> ~47ms, steady throughput
~59k -> ~65k env-steps/s.
@wlgys8
wlgys8 force-pushed the perf/fastsac-async-push-61 branch from 9861f22 to aa3bde2 Compare September 20, 2026 17:00
@wlgys8

wlgys8 commented Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

Copilot 最新 overview 提到 ring_capacity >= 2 契约会破坏 scripts/bench_fastsac_collector_inference.py(原来构造 SharedTransitionRing(1, ...))——已确认并在 aa3bde2 修复:该 bench 只用 _infer/sync_weights,从不 push ring,capacity=1 只是最小占位,改为 2 并加注释说明。其余上一轮意见均已 resolve(thread cap 那条见上方回复)。

@wlgys8
wlgys8 merged commit 4ec2de2 into main Sep 21, 2026
5 checks passed
@wlgys8
wlgys8 deleted the perf/fastsac-async-push-61 branch September 21, 2026 03:43
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.

perf(fastsac): async learner 进程抢 CPU 核,拖慢 collector 的 env.step perf(fastsac): async collector push 的拷贝冗余与同步阻塞

2 participants