feat(a5): add scheduler L2 phase telemetry - #1711
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 returns fanout traversal counts in non-profiling builds and records additional Resolve, Drain, DrainPrepare, DrainPublish, and Release phase timings under profiling configurations. ChangesScheduler profiling updates
Sequence Diagram(s)sequenceDiagram
participant SchedulerCompletion
participant SchedulerDispatch
participant PTO2SchedulerState
participant SchedulerPhaseProfiling
SchedulerCompletion->>PTO2SchedulerState: complete task
PTO2SchedulerState-->>SchedulerCompletion: fanout count or CompletionStats
SchedulerCompletion->>SchedulerPhaseProfiling: record Resolve and drain phases
SchedulerDispatch->>PTO2SchedulerState: complete dummy task
PTO2SchedulerState-->>SchedulerDispatch: resolved fanout count
SchedulerDispatch->>SchedulerPhaseProfiling: record Drain and Release phases
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_dispatch.cpp`:
- Around line 1192-1212: Instrument every deferred-release flush with the
`Release` phase timing and processed-count record, not only the loop near
`deferred_release_slot_states` in the shown scheduler path. Reuse a shared flush
helper if available, or apply equivalent timing around the capacity-triggered
flushes in `scheduler_completion.cpp`, this file’s earlier flush path, and the
final cleanup; preserve the existing release callback behavior and profiling
variants.
- Around line 1016-1028: Update handle_drain_mode and stage_sync_start_cores to
return the number of staged blocks, propagate that count through the drain path,
and pass it as the Drain phase record’s tasks_processed value in
chip_swimlane_aicpu_record_sched_phase. Keep drain_stage_wall exclusively as the
elapsed-time value.
In `@tests/ut/cpp/a5/test_wiring.cpp`:
- Around line 414-419: Update the profiling-enabled call to
PTO2SchedulerState::on_task_complete in the test around completion_stats to pass
the profiling thread index explicitly, using 0 as the test index; leave the
non-profiling call unchanged.
🪄 Autofix
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: b795e146-77a1-4fb5-b3d1-3ddc0bc070dc
📒 Files selected for processing (4)
src/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_dispatch.cpptests/ut/cpp/a5/test_wiring.cpp
25dacbf to
855eb37
Compare
855eb37 to
d019d69
Compare
ChaoZheng109
left a comment
ChaoZheng109
left a comment
There was a problem hiding this comment.
整体是一个合格的 a5←a2a3 DFX 对齐 back-port，范围清晰、带回归测试。两点想请作者确认（已作为 inline 评论）：Release 记录的发射位置与 a2a3 不一致，以及 flush_deferred_releases 只加在 a5 引入的两树结构分叉。
| #if SIMPLER_DFX | ||
| if (release_t0 != 0) { | ||
| chip_swimlane_aicpu_record_sched_phase( | ||
| thread_idx, ChipSwimlaneSchedPhaseKind::Release, release_t0, get_sys_cnt_aicpu(), |
There was a problem hiding this comment.
[需澄清] Release 发射位置与 a2a3 不一致，和"对齐 A2/A3"目标相悖。
把 Release 记录放进 flush_deferred_releases 内部后，4 个调用点会全部发射 Release：
completion.cpp:204（completion 内 cap 触发的 flush）scheduler_dispatch.cpp:1115（dummy cap 触发的 flush）scheduler_dispatch.cpp:1213（no-progress 空转路径）scheduler_dispatch.cpp:1289（最终 drain）
而 a2a3 只在 no-progress 空转路径这一处发射 Release（a2a3/.../scheduler_dispatch.cpp:1399）；completion 的 cap-flush、dummy cap-flush、final-drain 三处刻意不记录（a2a3 scheduler_completion.cpp 中 Release 出现次数为 0）。
后果:
- 与 PR 声明的"对齐 A2/A3"直接矛盾——a5 会在 a2a3 有意留白的位置多出
Releasebar，下一个做对齐的人可能当回归。 - cap 触发的 flush 发生在有进展的半热路径上（一次放最多 256 个 slot），在此发
Release会在Complete/Dummy外层 bar 内产生 a2a3 不会出现的嵌套/并列 span，可能干扰 swimlane 的时间包含关系。
建议二选一:
- 收敛到 a2a3 语义:仅在 no-progress 路径发
Release(把记录逻辑留在该调用点,而非塞进 helper);或 - 若这是有意增强(a5 额外覆盖 cap/final-drain 的释放),请在 PR 描述或此处注释里明确说明,以免被误判为偏离。
There was a problem hiding this comment.
已按 a2a3 的既有语义收敛：Release 只在 no-progress flush 路径发射；completion 容量 flush、dummy 容量 flush和final cleanup 仍执行释放，但不额外生成嵌套 Release bar。flush_deferred_releases() 现在只负责释放逻辑。修复已包含在 commit id：ea40e300 中
| inline constexpr int32_t PTO2_DEFERRED_RELEASE_CAP = 256; | ||
| } | ||
|
|
||
| void SchedulerContext::flush_deferred_releases( |
There was a problem hiding this comment.
[Consider] flush_deferred_releases 只加在 a5，引入两树结构分叉。
就 DRY 而言，把原本 4 处重复的 deferred-release 循环收敛成一个 helper 是改进。但 a2a3 目前仍是 4 处内联循环，本 PR 只在 a5 抽取 helper，会让两棵树在结构上更不一致——这与本 PR 声明的"对齐"方向以及 CLAUDE.md rule 10（两树尽量收敛）略有张力。
非阻塞项:若目标是对齐，理想做法是同一 helper 也落到 a2a3（可作后续小 PR）。此处仅提示，便于后续跟踪。
There was a problem hiding this comment.
已移除 A5-only 的 flush_deferred_releases helper，将四处 deferred-release 循环恢复为与 A2/A3 相同的内联结构，避免两棵 runtime 树新增结构分叉。后续若要统一抽象，会在 A2/A3 与 A5 中同步以独立重构处理。
25139c0 to
cd44b04
Compare
Record Resolve, Drain, DrainPrepare, DrainPublish, and Release work so A5 traces expose the same scheduler detail as A2/A3. Return fanout traversal counts and report staged-block and deferred-release counts through tasks_processed. Keep Release on the no-progress deferred flush path to match A2/A3. Add positive scene coverage for all five phases and document their scheduler-lane roles and workload semantics. Validation: - A5SIM chip-swimlane DFX scenes (4 passed) - A5SIM drain scene without profiling (1 passed) - pre-commit checks
cd44b04 to
6594e5c
Compare
2 participants
Summary
This PR extends A5 scheduler observability so chip-swimlane profiling can attribute scheduler-side work to the same sub-phases already exposed by A2/A3.
What changed
Resolvearound task completion and report the number of fanout consumers traversed.Drainaround successful sync-start drain staging and report the number of blocks staged by each scheduler thread.DrainPrepareandDrainPublishrecords with published-subtask counts.Releasework on the no-progress flush path, matching the A2/A3 emission point.on_task_complete()path.Tests and documentation
ResolveandReleasework.Drain,DrainPrepare, andDrainPublishall report consistent, non-zero workload counts.tasks_processedsemantics.Verification
halResMapsymbol; the PR CI hardware lane provides the authoritative onboard result.Relation
Related to #1582.