Add: protect args dump arena reuse with backpressure - #1662
Conversation
📝 WalkthroughWalkthroughThe PR adds payload publication and completion watermarks to dump buffers. AICPU arena reuse now waits for host acknowledgement. The host collector refreshes device state on demand and releases backpressure after all payloads are written. Tests cover A2A3, A5, and multi-arena flows. ChangesArgs dump arena backpressure
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AICPU
participant DumpBufferState
participant ArgsDumpCollector
AICPU->>DumpBufferState: publish payload count
AICPU->>DumpBufferState: wait for completed payload count
ArgsDumpCollector->>DumpBufferState: publish completed payload count
DumpBufferState-->>AICPU: allow arena reuse
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: 3
🧹 Nitpick comments (1)
src/common/platform/shared/host/args_dump_collector.cpp (1)
279-295: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftConsider copying only the newly-written arena span instead of the full arena on every collection.
Once
write_offsetexceedsai.size,bytes_to_copyis clamped to the full arena size on every call, so each buffer collection re-copies the entire per-thread arena (up to 128 MiB by default) rather than only the bytes written since the last refresh. WithPLATFORM_DUMP_BUFFERS_PER_THREAD = 8, this can multiply the actual host-device copy volume roughly 8x per arena lap. Track the last-copied offset per thread and copy only the delta (handling the wrap case) to reduce I/O on the a5rtMemcpy/memcpytransport.Please confirm whether the actual copy volume introduced by this per-collection full-arena refresh was measured against the prior full-shm-mirror-per-tick baseline it replaces.
🤖 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/host/args_dump_collector.cpp` around lines 279 - 295, Update the arena refresh logic around arenas_ and the write_offset calculation to retain a last-copied offset for each thread and copy only the newly written span, including correct wrap-around handling when the write cursor laps ai.size. Preserve the existing bounds checks and device-to-host copy behavior, and update the tracked offset after each successful refresh. Measure and report the resulting copy volume against the prior full-shm-mirror-per-tick baseline.
🤖 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/common/platform/shared/host/args_dump_collector.cpp`:
- Around line 286-289: Check the return value of profiling_copy_from_device in
the dump-buffer state flow before using state->arena_write_offset; on failure,
follow the existing error-handling behavior used by equivalent calls in
backpressure_release_ready() and stop processing rather than copying bytes from
a stale offset.
- Around line 615-650: Update ArgsDumpCollector::backpressure_release_ready() to
acknowledge each dump thread independently rather than gating all progress on
written_payload_count_ equaling the summed published_payload_count. Remove or
bypass the global-count check, and advance each thread’s completed_payload_count
when that thread’s published data has been written/read, preserving the existing
per-thread device copy and failure handling.
In `@tests/ut/cpp/a2a3/test_args_dump.cpp`:
- Around line 235-249: The host-simulation lambda copies arena, so its race
check reads a stale snapshot instead of the shared buffer. In
tests/ut/cpp/a2a3/test_args_dump.cpp lines 235-249, update the std::thread host
capture to capture arena by reference; apply the identical change in
tests/ut/cpp/a5/test_args_dump.cpp lines 235-249 so payload_preserved_before_ack
validates the live arena contents.
---
Nitpick comments:
In `@src/common/platform/shared/host/args_dump_collector.cpp`:
- Around line 279-295: Update the arena refresh logic around arenas_ and the
write_offset calculation to retain a last-copied offset for each thread and copy
only the newly written span, including correct wrap-around handling when the
write cursor laps ai.size. Preserve the existing bounds checks and
device-to-host copy behavior, and update the tracked offset after each
successful refresh. Measure and report the resulting copy volume against the
prior full-shm-mirror-per-tick baseline.
🪄 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: 9ac06d8a-bd04-4051-96d6-c535362218ab
📒 Files selected for processing (8)
docs/dfx/args-dump.mdsrc/common/platform/include/common/args_dump.hsrc/common/platform/include/host/args_dump_collector.hsrc/common/platform/shared/aicpu/args_dump_aicpu.cppsrc/common/platform/shared/host/args_dump_collector.cpptests/ut/cpp/a2a3/test_args_dump.cpptests/ut/cpp/a5/test_args_dump.cpptests/ut/cpp/common/test_args_dump_collector.cpp
- Publish and acknowledge payload watermarks before arena reuse - Track writer completion per AICPU thread to prevent cross-thread ACKs - Bound freeze-release and payload-ack waits with one timeout budget - Cover arena preservation, pop-gate semantics, and collector release
f7eb998 to
aec57ab
Compare
Summary
written[t] == published[t].Core flow
Testing
task-submit, with--dump-args 2, temporary 256 MiB/thread arena, and batch 32:truncated_args=0,dropped_records=0, anddropped_overwrite=0.Known capacity/throughput limit
With the default 128 MiB/thread arena and the full page-unroll batch 256 workload, the host completed 46 trigger/release pairs with no dropped or overwritten records, but cumulative dump/write waits exceeded the outer 45-second AICPU op-execute timeout. Increasing the arena to 256 MiB/thread and using batch 32 produced a complete passing run. The backpressure protocol protects correctness; very large dump workloads still need enough arena capacity and host write throughput to finish within the outer op timeout.