fix(runtime): avoid global drain for local early sync-start - #1558
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe scheduler now supports capacity-aware local staging for early ChangesSync-start staging flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant CoreTracker
participant Staging
participant Rendezvous
Scheduler->>CoreTracker: count_available_blocks
CoreTracker-->>Scheduler: available logical blocks
Scheduler->>Staging: stage_sync_start_cores
Staging-->>Scheduler: staged_blocks and running_cores
Scheduler->>Rendezvous: publish staged mask and running count
Rendezvous->>Rendezvous: retry after staging
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/st/a5/tensormap_and_ringbuffer/dfx/l2_swimlane/test_sync_start_early_local_owner.py`:
- Around line 94-106: Extend the assertions in the perf-phase validation near
phase_counts to verify that the local-owner path contains no records with phases
“drain”, “drain_prepare”, or “drain_publish”, mirroring the A2A3 test behavior.
Keep the existing early_dispatch count assertion and include the artifact path
in any new failure message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 09dcf435-c281-4e1f-ad67-f88dabc62f8d
📒 Files selected for processing (27)
src/a2a3/runtime/host_build_graph/docs/RUNTIME_LOGIC.mdsrc/a2a3/runtime/host_build_graph/runtime/scheduler/pto_scheduler.hsrc/a2a3/runtime/host_build_graph/runtime/scheduler/scheduler_completion.cppsrc/a2a3/runtime/host_build_graph/runtime/scheduler/scheduler_context.hsrc/a2a3/runtime/host_build_graph/runtime/scheduler/scheduler_dispatch.cppsrc/a2a3/runtime/host_build_graph/runtime/scheduler/scheduler_types.hsrc/a2a3/runtime/tensormap_and_ringbuffer/docs/RUNTIME_LOGIC.mdsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/pto_scheduler.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_completion.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_context.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_dispatch.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_types.hsrc/a5/runtime/tensormap_and_ringbuffer/docs/RUNTIME_LOGIC.mdsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/pto_scheduler.hsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_completion.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_context.hsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_dispatch.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_types.hsrc/common/platform/include/common/l2_swimlane_profiling.htests/st/a2a3/tensormap_and_ringbuffer/dfx/l2_swimlane/kernels/orchestration/sync_start_early_local_owner_orch.cpptests/st/a2a3/tensormap_and_ringbuffer/dfx/l2_swimlane/test_sync_start_early_local_owner.pytests/st/a5/tensormap_and_ringbuffer/dfx/l2_swimlane/kernels/orchestration/sync_start_early_local_owner_orch.cpptests/st/a5/tensormap_and_ringbuffer/dfx/l2_swimlane/test_sync_start_early_local_owner.pytests/ut/cpp/a2a3/test_scheduler_state.cpptests/ut/cpp/a2a3/test_wiring.cpptests/ut/cpp/a5/test_scheduler_state.cpptests/ut/cpp/a5/test_wiring.cpp
972374b to
5243fdc
Compare
ChaoZheng109
left a comment
There was a problem hiding this comment.
文档一致性:重命名后有 2 处 drain_stage_cores 旧名残留
本 PR 已将运行时函数 drain_stage_cores 重命名为 stage_sync_start_cores(三棵树的实现与调用点均已同步,👍)。但 spmd_sync_start_mix_spill 测试目录下仍有两处散文注释引用旧函数名,建议在本 PR 一并更新,避免遗留失效引用(参见 .claude/rules/doc-consistency.md §1:重命名后需审计全仓引用):
tests/st/a2a3/tensormap_and_ringbuffer/spmd_sync_start_mix_spill/kernels/orchestration/spmd_sync_start_mix_spill_orch.cpp:20* cores' gated pending slots (drain_stage_cores takes the to_pending=true split path,tests/st/a2a3/tensormap_and_ringbuffer/spmd_sync_start_mix_spill/test_spmd_sync_start_mix_spill.py:14(drain_stage_cores to_pending=true, mix_cluster_idle_core_count=1/cluster + Case 3.3 promote for
这两个文件不在本 PR 的 diff 内,故无法作为行内评论,统一在此说明。属于轻微(minor)问题,不阻塞合入。
其余部分 LGTM:Case A 逻辑在 a2a3 host_build_graph / a2a3 tmr / a5 tmr 三棵树逐字一致,发布顺序严格镜像 drain finalize,maybe_rendezvous_ring 内存序已加固,单元/scene 测试覆盖到位(scene 断言非 owner 线程无 drain 相位)。
c6a5ad8 to
6f7141f
Compare
|
@ChaoZheng109 感谢指出。两处散文注释中的旧函数名已在最新提交
同时已按文档一致性规则重新执行全仓搜索, |
e30e2e2 to
4998ecd
Compare
Stage an early sync-start cohort directly on its owning scheduler when that scheduler's running and pending slots can hold every block. Keep the stop-the-world drain as the fallback when capacity must be gathered across scheduler threads, and preserve the one-shot producer/stager rendezvous. Share available-block accounting across AIC, AIV, and MIX placement, cover launch ownership and pending-slot cases with unit tests, and add A2A3/A5 regression scenes that require early_dispatch without a drain when the cohort fits locally. Fixes hw-native-sys#1548
Summary
This PR fixes #1548 by adding a local-owner fast path for early
require_sync_startcohorts. When one scheduler's tracker can place every block using its local idle running slots and free pending slots, that scheduler stages the complete cohort locally. Cohorts that do not fit on one owner continue to use the existing global stop-the-world drain.The active end-to-end fast path is implemented and validated for:
tensormap_and_ringbuffertensormap_and_ringbufferA2A3
host_build_graphcarries the same scheduler-side capacity and staging machinery for implementation parity, but itspropagate_dispatch_faninpath is currently a stub. It therefore does not populate the early queues, and the HBG fast path remains dormant rather than being an active end-to-end behavior.Problem
Early
sync_startstaging must be all-or-nothing. Partially staging a gated cohort can strand blocks before the rendezvous reachesblock_num, so the previous implementation conservatively routed every earlysync_startcohort through the global drain protocol.That was unnecessarily expensive when the scheduler that claimed the cohort already had enough local capacity. In the representative 8-block AIV case from #1548, one scheduler could hold the entire cohort, but all scheduler threads still had to stop, acknowledge the drain generation, scan capacity, prepare blocks, and publish them globally.
The minimal regression shows the behavioral difference clearly:
drain_prepare=1,drain_publish=1,drain=1, with 8/8 blocks published by the drain.early_dispatch=8,drain=0, with all blocks staged by one local owner.Root cause
The Tier-0 early
sync_startpath claimed a per-task owner and immediately attemptedenter_drain_mode(...). It had no exact local-capacity check and no local all-or-nothing staging branch, even though the existing tracker state already described idle and pending placement capacity.What changed
count_available_blocks(...)accounting for AIC, AIV, and MIX shapes.stage_sync_start_cores(...)so local and global paths use the same placement semantics.logical_block_numlocally.running_slot_countis seeded from directly staged running cores, then the producer/stager rendezvous is retried.early_dispatchscheduler phase on A5.Correctness and concurrency guarantees
The fast path keeps the same safety properties as the global path:
logical_block_num;Test coverage
Unit tests cover both A2A3 and A5 scheduler implementations, including:
Scene regressions now cover three topologies on both the A2A3 and A5
tensormap_and_ringbufferruntimes:The pending-only scene uses blocker start markers to ensure the consumer is submitted after blocker ACKs are observable. Its trace assertion also proves local staging finishes before any blocker releases a running slot, so the test exercises real pending-slot placement rather than an idle fallback.
Validation results
early_dispatchblocksAdditional representative
l3_moevalidation passed golden checks on both ranks and showed no drain. A same-card 100-round ABBA comparison measured a median of 750.7 us for this PR versus 753.3 us for clean main (about -0.35%, within measurement noise), with no observed performance regression.Relevant C++ scheduler-state and wiring tests pass for A2A3 and A5, and all repository pre-commit checks pass (
clang-format,clang-tidy,cpplint, Ruff, Pyright, header/license checks, and whitespace checks).Expected impact
In the active A2A3 and A5
tensormap_and_ringbufferearly-propagation paths, workloads whose earlysync_startcohort fits one scheduler avoid unnecessary cross-thread coordination and stop-the-world drain overhead. Larger cohorts retain the existing global aggregation behavior. There is no public API change.A2A3
host_build_graphcontains the scheduler-side implementation but does not currently expose this optimization end to end because early producer propagation is disabled there; no active HBG performance benefit is claimed by this PR.The exact historical
fused_pre_norm_cce.pyworkload referenced by #1548 is not present on current publicmain, so the PR uses a minimal before/after regression to reproduce the scheduler condition and representativel3_moeruns for compatibility and performance validation.Fixes #1548