Reduce hot-path logging footprint - #1437
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
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:
📝 WalkthroughWalkthroughThis change reduces informational logging across AICPU executors, host runtime builders, schedulers, PMU collectors, and profiling collectors. Core execution, buffer handling, synchronization, cleanup, and error paths remain active, with selected dispatch and accounting logic adjusted. ChangesRuntime diagnostics reduction
Estimated code review effort: 3 (Moderate) | ~20 minutes 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: 1
🧹 Nitpick comments (2)
src/common/platform/shared/aicpu/l2_swimlane_collector_aicpu.cpp (1)
824-850: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead logging scaffolding.
Drop the unusedconst char *kind_labelparameter and call-site argument, remove the emptyif (buf != nullptr) {}branch, and invert the flush check toif (rc != 0)so the success path isn’t empty.🤖 Prompt for 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. In `@src/common/platform/shared/aicpu/l2_swimlane_collector_aicpu.cpp` around lines 824 - 850, Remove the unused kind_label parameter from acquire_phase_slot and all of its call sites, delete the empty if (buf != nullptr) branch after pop_free, and update the flush result check to use if (rc != 0), leaving the success path without an empty branch.src/a2a3/runtime/host_build_graph/host/runtime_maker.cpp (1)
998-1002: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove empty branches left by log deletion.
The removed success logs leave an empty
elseblock and an emptyif (kernel_count > 0)block. Delete both; the surrounding error handling and unconditional cleanup call already provide the required behavior.Proposed cleanup
if (copy_rc != 0) { LOG_ERROR("Failed to copy tensor %d from device: %d", i, copy_rc); rc = copy_rc; - } else { } @@ - if (kernel_count > 0) {} runtime->clear_registered_kernels();Also applies to: 1022-1028
🤖 Prompt for 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. In `@src/a2a3/runtime/host_build_graph/host/runtime_maker.cpp` around lines 998 - 1002, Remove the empty else branch following the copy_rc error handling, and remove the empty if (kernel_count > 0) block in the same runtime graph-building flow. Preserve the existing error handling and keep the cleanup call unconditional.
🤖 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/host_build_graph/aicpu/aicpu_executor.cpp`:
- Line 1015: Update run() to capture the return value from
resolve_and_dispatch(), then preserve required buffer flushing and AICore
shutdown before returning that result. Ensure finished_count_ is not incremented
and finished_ is not published when resolve_and_dispatch() reports failure;
successful execution should retain the existing completion behavior.
---
Nitpick comments:
In `@src/a2a3/runtime/host_build_graph/host/runtime_maker.cpp`:
- Around line 998-1002: Remove the empty else branch following the copy_rc error
handling, and remove the empty if (kernel_count > 0) block in the same runtime
graph-building flow. Preserve the existing error handling and keep the cleanup
call unconditional.
In `@src/common/platform/shared/aicpu/l2_swimlane_collector_aicpu.cpp`:
- Around line 824-850: Remove the unused kind_label parameter from
acquire_phase_slot and all of its call sites, delete the empty if (buf !=
nullptr) branch after pop_free, and update the flush result check to use if (rc
!= 0), leaving the success path without an empty branch.
🪄 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: ae8edc52-c723-4975-b4c3-9681b355738b
📒 Files selected for processing (20)
src/a2a3/platform/shared/aicpu/pmu_collector_aicpu.cppsrc/a2a3/runtime/host_build_graph/aicpu/aicpu_executor.cppsrc/a2a3/runtime/host_build_graph/host/runtime_maker.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/aicpu/aicpu_executor.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/host/runtime_maker.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_completion.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_dispatch.cppsrc/a5/platform/shared/aicpu/pmu_collector_aicpu.cppsrc/a5/runtime/host_build_graph/aicpu/aicpu_executor.cppsrc/a5/runtime/host_build_graph/host/runtime_maker.cppsrc/a5/runtime/tensormap_and_ringbuffer/aicpu/aicpu_executor.cppsrc/a5/runtime/tensormap_and_ringbuffer/host/runtime_maker.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_completion.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_dispatch.cppsrc/common/platform/shared/aicpu/args_dump_aicpu.cppsrc/common/platform/shared/aicpu/dep_gen_collector_aicpu.cppsrc/common/platform/shared/aicpu/l2_swimlane_collector_aicpu.cppsrc/common/platform/shared/aicpu/scope_stats_collector_aicpu.cpp
💤 Files with no reviewable changes (13)
- src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_dispatch.cpp
- src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_dispatch.cpp
- src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_completion.cpp
- src/common/platform/shared/aicpu/dep_gen_collector_aicpu.cpp
- src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_completion.cpp
- src/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp
- src/a2a3/runtime/host_build_graph/aicpu/aicpu_executor.cpp
- src/a2a3/runtime/tensormap_and_ringbuffer/host/runtime_maker.cpp
- src/a5/runtime/host_build_graph/host/runtime_maker.cpp
- src/a5/runtime/tensormap_and_ringbuffer/host/runtime_maker.cpp
- src/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_cold_path.cpp
- src/a2a3/runtime/tensormap_and_ringbuffer/aicpu/aicpu_executor.cpp
- src/a5/runtime/tensormap_and_ringbuffer/aicpu/aicpu_executor.cpp
| LOG_INFO_V0("Thread %d: Runtime has %d tasks", thread_idx, runtime->get_task_count()); | ||
| int completed = resolve_and_dispatch(*runtime, thread_idx, cur_thread_cores, thread_cores_num_[thread_idx]); | ||
| LOG_INFO_V0("Thread %d: Executed %d tasks from runtime", thread_idx, completed); | ||
| resolve_and_dispatch(*runtime, thread_idx, cur_thread_cores, thread_cores_num_[thread_idx]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate resolve_and_dispatch() failures.
resolve_and_dispatch() returns -1 on timeout/stuck-state paths, but run() discards the result and continues to mark the executor finished before returning 0. This masks failed execution as success.
Capture the return value and propagate it after required buffer flushing and AICore shutdown, before incrementing finished_count_ or publishing finished_.
Proposed fix
- resolve_and_dispatch(*runtime, thread_idx, cur_thread_cores, thread_cores_num_[thread_idx]);
+ int dispatch_rc = resolve_and_dispatch(*runtime, thread_idx, cur_thread_cores, thread_cores_num_[thread_idx]);
// ... flush profiling buffers ...
int rc = shutdown_aicore(runtime, thread_idx, cur_thread_cores);
if (rc != 0) {
return rc;
}
+ if (dispatch_rc < 0) {
+ return dispatch_rc;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resolve_and_dispatch(*runtime, thread_idx, cur_thread_cores, thread_cores_num_[thread_idx]); | |
| int dispatch_rc = resolve_and_dispatch(*runtime, thread_idx, cur_thread_cores, thread_cores_num_[thread_idx]); | |
| // ... flush profiling buffers ... | |
| int rc = shutdown_aicore(runtime, thread_idx, cur_thread_cores); | |
| if (rc != 0) { | |
| return rc; | |
| } | |
| if (dispatch_rc < 0) { | |
| return dispatch_rc; | |
| } |
🤖 Prompt for 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.
In `@src/a5/runtime/host_build_graph/aicpu/aicpu_executor.cpp` at line 1015,
Update run() to capture the return value from resolve_and_dispatch(), then
preserve required buffer flushing and AICore shutdown before returning that
result. Ensure finished_count_ is not incremented and finished_ is not published
when resolve_and_dispatch() reports failure; successful execution should retain
the existing completion behavior.
0e808c3 to
c1128ee
Compare
c1128ee to
e60cfac
Compare
2 participants
Summary
AICPU target paths no longer emit V0–V8 INFO logs.
Validation
.venv/bin/pip install --no-build-isolation -e .DenseFanoutFaninCloses #1428