refactor: extract AICPU device profiler engine - #1276
ChaoZheng109 merged 2 commits into
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:
📝 WalkthroughWalkthroughIntroduces a shared ChangesAICPU Device Profiler Engine Extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Collector as AICPU Collector
participant Engine as DeviceProfilerEngine
participant Ready as Ready Queue
participant Free as Free Queue
Collector->>Engine: switch_buffer(ctx, state)
Engine->>Ready: enqueue_ready(full buffer)
alt enqueue succeeds
Engine->>Engine: on_current_cleared()
else ready queue full
Engine->>Engine: on_enqueue_dropped()
end
Engine->>Free: pop_free(next_seq)
alt free buffer available
Engine->>Collector: on_switch_complete(new buffer)
else no replacement
Engine->>Collector: on_no_replacement()
end
Possibly related issues
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 |
![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 unified AICPU-side producer algorithm layer, DeviceProfilerEngine, in profiler_device_engine.h and refactors various profiling collectors (DepGen, L2Swimlane, PMU, ScopeStats, and TensorDump) across the a2a3 and a5 platforms to use it, reducing code duplication. Feedback on the new engine highlights a performance concern in the tight spin loops of wait_for_ready_queue_space and wait_for_free_queue_entry, where calling get_sys_cnt_aicpu() on every iteration can cause high CPU overhead due to expensive MMIO reads; gating these checks to run periodically is recommended.
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.
| const uint64_t start = get_sys_cnt_aicpu(); | ||
| do { | ||
| uint32_t current_tail = header->queue_tails[thread_idx]; | ||
| uint32_t current_head = header->queue_heads[thread_idx]; | ||
| uint32_t next_tail = (current_tail + 1) % Module::kReadyQueueSize; | ||
| if (next_tail != current_head) { | ||
| *tail_out = current_tail; | ||
| *head_out = current_head; | ||
| return true; | ||
| } | ||
| if (get_sys_cnt_aicpu() - start >= Module::kBackpressureWaitCycles) { | ||
| break; | ||
| } | ||
| } while (true); |
There was a problem hiding this comment.
Reading system counters via get_sys_cnt_aicpu() on every iteration of a tight spin loop can introduce significant CPU overhead and latency due to expensive MMIO reads. It is highly recommended to gate these checks to run periodically (e.g., every 1024 spins) to minimize overhead.
const uint64_t start = get_sys_cnt_aicpu();
uint32_t spin_count = 0;
do {
uint32_t current_tail = header->queue_tails[thread_idx];
uint32_t current_head = header->queue_heads[thread_idx];
uint32_t next_tail = (current_tail + 1) % Module::kReadyQueueSize;
if (next_tail != current_head) {
*tail_out = current_tail;
*head_out = current_head;
return true;
}
if (++spin_count % 1024 == 0) {
if (get_sys_cnt_aicpu() - start >= Module::kBackpressureWaitCycles) {
break;
}
}
} while (true);References
- Avoid reading system counters (which can be expensive MMIO reads) or performing complex structural checks on every iteration of a tight spin loop. Instead, gate these checks to run periodically (e.g., every 1024 spins) to minimize CPU overhead and latency impact.
There was a problem hiding this comment.
Addressed in eac1111 by polling the backpressure timeout every 1024 spins instead of reading get_sys_cnt_aicpu() on every wait-loop iteration. The zero-wait-cycle case still exits immediately.
| const uint64_t start = get_sys_cnt_aicpu(); | ||
| do { | ||
| uint32_t head = free_queue->head; | ||
| uint32_t tail = free_queue->tail; | ||
| if (head != tail) { | ||
| *head_out = head; | ||
| *tail_out = tail; | ||
| rmb(); // acquire: order the tail read above before the caller's buffer_ptrs read | ||
| return true; | ||
| } | ||
| if (get_sys_cnt_aicpu() - start >= Module::kBackpressureWaitCycles) { | ||
| break; | ||
| } | ||
| } while (true); |
There was a problem hiding this comment.
Reading system counters via get_sys_cnt_aicpu() on every iteration of a tight spin loop can introduce significant CPU overhead and latency due to expensive MMIO reads. It is highly recommended to gate these checks to run periodically (e.g., every 1024 spins) to minimize overhead.
const uint64_t start = get_sys_cnt_aicpu();
uint32_t spin_count = 0;
do {
uint32_t head = free_queue->head;
uint32_t tail = free_queue->tail;
if (head != tail) {
*head_out = head;
*tail_out = tail;
rmb(); // acquire: order the tail read above before the caller's buffer_ptrs read
return true;
}
if (++spin_count % 1024 == 0) {
if (get_sys_cnt_aicpu() - start >= Module::kBackpressureWaitCycles) {
break;
}
}
} while (true);References
- Avoid reading system counters (which can be expensive MMIO reads) or performing complex structural checks on every iteration of a tight spin loop. Instead, gate these checks to run periodically (e.g., every 1024 spins) to minimize CPU overhead and latency impact.
There was a problem hiding this comment.
Addressed in eac1111 with the same 1024-spin gated timeout polling in wait_for_free_queue_entry(), avoiding a get_sys_cnt_aicpu() read on every tight-loop iteration while keeping the memory barrier behavior unchanged.
ab7e64e to
13b39eb
Compare
|
|
🐮 |
| if (Module::kBackpressureWaitCycles == 0) { | ||
| break; | ||
| } | ||
| if ((++spin_count & kCounterPollSpinMask) != 0) { |
There was a problem hiding this comment.
Undocumented behavior change: the new kCounterPollSpinMask alters the bounded-wait, contrary to the PR's "preserve existing behavior" claim.
Both wait_for_ready_queue_space and wait_for_free_queue_entry now gate the timeout check behind (++spin_count & kCounterPollSpinMask) != 0 continue — so get_sys_cnt_aicpu() is only sampled once per 1024 spins. The four common collectors (ScopeStats / DepGen / TensorDump / PMU) and the old L2 path all checked get_sys_cnt_aicpu() - start >= kBackpressureWaitCycles on every iteration and had no poll-mask before this PR (verified against merge-base). So this is a genuine, new behavior for all of them.
I don't think it's a correctness issue: the timeout overshoot is bounded to ~1024 cheap head/tail reads, and cutting the sys-cnt sampling rate is almost certainly a perf win on the AICPU. But two things:
- It's not "preserving the existing barrier/timeout behavior" as the description states — worth calling out explicitly so a future reader doesn't treat it as accidental drift.
- Same note applies to the
rmb()thatpop_freenow always issues after readingbuffer_ptrs[head]: ScopeStats/DepGen/TensorDump/PMU previously had no such barrier there (only old L2 did). It's a strengthening (safe), but it's another silent semantic unification.
Could you add a one-line comment on kCounterPollSpinMask (and on that rmb()) noting these are deliberate, so the intent is captured? Alternatively, if bit-for-bit behavior preservation was the goal, drop the poll-mask and keep per-iteration timeout checks.
There was a problem hiding this comment.
Thanks for calling this out. I chose the bit-for-bit timeout-preservation path here: removed kCounterPollSpinMask/spin_count and restored per-iteration get_sys_cnt_aicpu() timeout checks in both wait loops, since the backpressure budget is only ~20us. I kept the extra rmb() after buffer_ptrs[head] and added a comment marking it as an intentional acquire-strengthening before advancing free_queue.head.
Introduce a shared DeviceProfilerEngine for AICPU-side profiling buffer handoff and switch logic. Migrate ScopeStats, DepGen, TensorDump, PMU, and the standard L2Swimlane AICPU task path while keeping subsystem-specific record fill, flush, and L2 AICore rotation semantics local. Update profiling framework docs for the new device-side layer. Throttle wait-loop timeout checks to avoid reading get_sys_cnt_aicpu() on every spin.
13b39eb to
fa09fee
Compare
refactor: extract AICPU device profiler engine Introduce a shared DeviceProfilerEngine for AICPU-side profiling buffer handoff and switch logic. Migrate ScopeStats, DepGen, TensorDump, PMU, and the standard L2Swimlane AICPU task path while keeping subsystem-specific record fill, flush, and L2 AICore rotation semantics local. Update profiling framework docs for the new device-side layer. Throttle wait-loop timeout checks to avoid reading get_sys_cnt_aicpu() on every spin.
2 participants
Summary
Fixes #1247.
This PR introduces a shared AICPU-side profiling device engine,
DeviceProfilerEngine<Module>, as the device-side counterpart to the hostProfilerAlgorithms<Module>pattern. The goal is not just to extract low-level wait helpers; it is to centralize the common producer-side enqueue/pop/switch buffer operation layer that had been duplicated across AICPU collectors.The shared engine now owns the common ready-queue handoff, free-queue pop/install, and current-buffer switch flow. Each collector keeps only its subsystem-specific record fill, flush/finalize, state shape, and local hooks.
What Changed
src/common/platform/include/aicpu/profiler_device_engine.h.docs/profiling-framework.mdto document the new device-side profiling layer.Design Notes
DeviceProfilerEngine<Module>is intentionally header-only and trait-driven, matching the existing host-side design style. The module trait supplies the collector-specific types, queue sizes, wait limits, pointer/sequence accessors, ready-entry writer, and event hooks.The engine covers the shared flow:
L2Swimlane is only partially normalized by design. Its AICPU task buffer path now uses the shared engine, while phase-pool switching/recovery and AICore rotation remain local because they have different sequence, retry, drop, and AICore-visible head semantics. Forcing those paths into the generic engine would make the common abstraction less clear and risk changing L2-specific behavior.
Validation
Build and static checks:
python -m pip install --no-build-isolation -e .cmake --build /data/jinzongquan/simpler/build/ut_cpp_issue1253 -j2git diff --checkUnit tests:
test_a2a3_orchestrator_fanintest_a5_orchestrator_fanintest_scope_stats_collectortest_a2a3_scope_stats_collectora2a3 onboard ST:
--dump-args 1,--dump-args 2, and--dump-args 3a5sim ST:
Known validation note:
--enable-l2-swimlane 1/2/3/4on this branch.--enable-l2-swimlane 1scenario was reproduced on a cleanmainworktree, so this appears to be a pre-existing main-branch issue rather than a regression from this PR.Risk / Follow-up