perf(l2-swimlane): capture orchestrator phase records per-scope (depth<=1), a2a3 + a5 (Swimlane L4 in-window overhead ~9% -> ~0%) - #1382
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds host-configured orchestrator scope depth to shared-memory initialization, supports per-scope timing records in profiling and DFX paths, and documents measurements leading to a depth-limited capture policy. ChangesOrchestrator scope profiling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Host
participant SharedHeader
participant AICPU
participant Orchestrator
participant Recorder
Host->>SharedHeader: Set orch_phase_scope_depth
SharedHeader->>AICPU: Initialize scope-depth configuration
AICPU->>Orchestrator: Provide configured depth
Orchestrator->>Recorder: Emit scope timing record
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a runtime-configurable orchestrator phase-record granularity (orch_phase_scope_depth) to mitigate L2 swimlane L4 orchestrator overhead, implementing a depth-limit approach based on performance investigations. Feedback on the changes highlights potential out-of-bounds write and read vulnerabilities in pto_orchestrator.cpp when indexing g_orch_scope_start_ts with orch->scope_stack_top in begin_scope and end_scope, and recommends adding defensive bounds checks against PTO2_MAX_SCOPE_DEPTH.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_orchestrator.cpp`:
- Around line 675-684: The orchestration phase recording in
l2_swimlane_aicpu_record_orch_phase passes scope_stack_top as an unencoded
task_id, violating the PTO2 cross-view correlation contract. Update this
scope-record path to use a distinct, full PTO2 encoding with the appropriate
ring identifier and local scope value, or route scopes through a dedicated API;
ensure downstream parsing can distinguish scope records from task records.
- Around line 621-626: Align all accesses to the DFX-only fields
orch_phase_scope_depth and l2_swimlane_level with SIMPLER_DFX: update the guards
around the scope-start logic at
src/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_orchestrator.cpp lines
621-626 and 675-684, guard the orch_phase_scope_depth access at line 147, and
ensure CYCLE_COUNT_START and CYCLE_COUNT_ORCH_SUBMIT_RECORD at lines 157-169
only evaluate these fields under SIMPLER_DFX.
In `@src/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_orchestrator.h`:
- Around line 101-103: Remove the configurable orch_phase_scope_depth knob and
its associated SIMPLER_ORCH_PHASE_SCOPE_DEPTH environment-variable and
get_orch_phase_scope_depth() plumbing. Define and use a fixed constant such as
kOrchPhaseMaxDepth = 1 in the orchestration phase-scope logic, preserving
per-submit behavior only where the hardcoded depth condition requires it.
In `@src/common/platform/shared/aicpu/l2_swimlane_collector_aicpu.cpp`:
- Line 280: Reset g_orch_phase_scope_depth to -1 alongside the other
launch-state resets before checking g_platform_l2_swimlane_base. Preserve the
existing header assignment afterward so a valid s_l2_swimlane_header overwrites
the reset value.
In `@src/common/platform/shared/host/l2_swimlane_collector.cpp`:
- Around line 214-217: Replace the unchecked std::atoi parsing of
SIMPLER_ORCH_PHASE_SCOPE_DEPTH in the orchestrator phase-depth initialization
with checked parsing that requires the entire value to be a valid signed integer
within int32_t range. Preserve -1 as the default and for invalid input, or fail
initialization with a clear diagnostic; do not allow malformed values to become
depth 0.
🪄 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
Run ID: 820b956a-436e-4ec8-9997-64ea82b72c61
📒 Files selected for processing (9)
docs/investigations/2026-07-l2-swimlane-orch-overhead-tracr-defer.mddocs/investigations/README.mdsrc/a2a3/runtime/tensormap_and_ringbuffer/aicpu/aicpu_executor.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_orchestrator.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_orchestrator.hsrc/common/platform/include/aicpu/l2_swimlane_collector_aicpu.hsrc/common/platform/include/common/l2_swimlane_profiling.hsrc/common/platform/shared/aicpu/l2_swimlane_collector_aicpu.cppsrc/common/platform/shared/host/l2_swimlane_collector.cpp
noabauma
changed the title
81805d9 to
3c0eb46
Compare
noabauma
changed the title
|
The CI is failing due to a mismatch in the golden score. Nothing which this PR is even touching... Please rerun it. |
3c0eb46 to
89474eb
Compare
…a3 + a5) L4 orchestrator profiling recorded one swim-lane span per submit_task() / alloc_tensors(), which put a per-task write on the orchestrator critical path (~+9% in-window on paged_attention). Record instead one span per orchestration scope at nesting depth <= 1: nearly all scopes nest deeper, so this keeps the top-level phase structure while cutting the in-window overhead to ~0% (Case2: +9.6% -> +1.3%, measured a2a3). The per-sub-step cumulative counters are unaffected. The per-submit record path (CYCLE_COUNT_ORCH_SUBMIT_RECORD and its g_orch_submit_idx / _submit_start_ts scaffolding) is removed outright rather than left as a no-op. The scope hooks read l2_swimlane_level, a SIMPLER_DFX-only field, so they are guarded with SIMPLER_DFX; a static_assert bounds the depth cap within g_orch_scope_start_ts. Applied identically to the a2a3 and a5 tensormap_and_ringbuffer orchestrators. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
89474eb to
04b352d
Compare
noabauma
changed the title
|
Concern: this trades away a DFX capability to shave an overhead that a lower profiling level already avoids. The ~9% in-window cost this PR targets is exclusive to the top profiling level ( What this change gives up in exchange is not free: it removes the per-task submit-timing granularity. Previously each The net effect is a worse trade on both ends: to lower overhead, a lower profiling level already achieves zero cost with no loss elsewhere; to keep per-task orchestrator detail, the pre-change behavior is required. The post-change top level only offers a coarse per-scope overview in between, at the cost of permanently reducing our DFX capture granularity — so this optimization is not worth the capability we lose for it. |
2 participants
Summary
L4 swimlane recorded one orchestrator phase span per
submit_task()/alloc_tensors(), putting a per-task write on the orchestrator critical path(~+9% in-window overhead on paged_attention). This records instead one span
per orchestration scope at nesting depth ≤ 1. Nearly all scopes nest deeper,
so this keeps the top-level phase structure while cutting the in-window overhead
to ≈0%.
The per-submit record path (
CYCLE_COUNT_ORCH_SUBMIT_RECORDand itsg_orch_submit_idx/_submit_start_tsscaffolding) is removed outright,not left as a no-op. Applied identically to the a2a3 and a5
tensormap_and_ringbufferorchestrators. The per-sub-step cumulative counters(
g_orch_*_cycle) are unaffected.Benchmark (a2a3, in-window
.orch, vs--enable-l2-swimlane-off idle)The win scales with how deeply a workload nests its scopes (near-total on the
big attention/decode kernels, negligible-but-harmless on trivial ones). With the
per-submit record fully removed, Case2 L4 is now within noise of idle.
STRACE device windows — Case2,
--rounds 1, median of 10 runs (devices 4–7)device_wall(depth 2)graph_build(depth 3)orch(depth 3)sched(depth 3)Enabling L4 inflates the orchestration window ~6.6% and the whole device run
~7% on
main; on this branch it stays within noise (~0%) across all windows.Trade-off
Orchestrator capture is now always per-scope depth≤1 — the previous per-task
(deeper-scope) orch detail is no longer produced. That detail was the source of
the overhead and is rarely used; the depth constant (
kOrchPhaseScopeDepth) isa one-line bump if a workload ever needs it, and a
static_assertbounds the capwithin
g_orch_scope_start_ts.Testing
profiling-flags-smokecombos(
SIMPLER_DFX=0,SIMPLER_ORCH_PROFILING,SIMPLER_SCHED_PROFILING,SIMPLER_TENSORMAP_PROFILING, and their unions).l2_swimlanescene test passes (a5 orch-phase path exercised).in-window.
main;st-onboard-a5exercises the a5 path on realsilicon in CI.