Repository navigation
Fix: make profiler host-ready hand-off lossless - #1658
Conversation
Retain ownership after a device-ready entry is popped and wait for collector capacity instead of retiring the buffer when the host-ready ring is full. Keep collectors alive until execution completes so final-drain producers always have a consumer, and cover the blocking hand-off, FIFO order, and collector liveness with regression tests.
📝 WalkthroughWalkthroughThe ready-queue handoff now blocks when the host queue is full. Queue state changes notify waiting producers and consumers. Collectors retain buffers through backpressure and continue after idle-timeout diagnostics until execution completes. Tests and profiling documentation cover these behaviors. ChangesReady-queue backpressure and collector lifetime
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.
🧹 Nitpick comments (2)
src/common/platform/include/host/buffer_pool_manager.h (1)
611-663: 🩺 Stability & Availability | 🔵 TrivialBlocking hand-off design is correct; no lost-wakeup or cross-wake ambiguity found.
The epoch capture happens after
push()/try_pop_ready()fails, but the predicate always re-checks!shard.queue.full()/!shard.queue.empty()directly, so even if the counterpart already advancedstate_epochbefore the epoch snapshot, the direct state check still unblocks the wait. Sincefull()andempty()are mutually exclusive forCapacity > 0, a producer wait and a consumer wait can never both be pending on the samecvat once, sonotify_one()cannot wake the wrong side.One trade-off worth flagging for operational awareness:
wait_push_to_readyhas no upper bound. IfDerived::on_buffer_collected(invoked synchronously from the collector'sconsume()) ever stalls — for example on blocked I/O — this loop blocks forever, andstop()'s join onmgmt_drain_threads_would hang indefinitely since nothing here checksmgmt_running_as an escape path. This appears to be an accepted trade-off given the PR's explicit goal of a lossless hand-off for any finite burst the collector can eventually drain, so no code change is requested. Consider whether the collector-side idle-timeout log (inprofiler_base.h) is wired to any operational alerting, since it is the only visible signal of a drain thread being stuck this way in production.🤖 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/include/host/buffer_pool_manager.h` around lines 611 - 663, No code change is requested. Retain the current blocking behavior in wait_push_to_ready, including its lossless hand-off semantics; only consider operationally wiring the collector idle-timeout signal from profiler_base.h to alerting.src/common/platform/include/host/profiler_base.h (1)
1226-1235: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDowngrade the idle-timeout log to
LOG_WARN.This timeout is diagnosed as a hang warning where the collector stays alive, but the call still routes through
LOG_ERROR. SinceLOG_WARNis available in this codebase, use it here to match the non-fatal semantics and avoid log-monitoring alert noise for a recovered condition.🤖 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/include/host/profiler_base.h` around lines 1226 - 1235, Change the idle-timeout log in the collector monitoring block from LOG_ERROR to LOG_WARN while preserving its message, arguments, and one-time detector reset behavior.
🤖 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.
Nitpick comments:
In `@src/common/platform/include/host/buffer_pool_manager.h`:
- Around line 611-663: No code change is requested. Retain the current blocking
behavior in wait_push_to_ready, including its lossless hand-off semantics; only
consider operationally wiring the collector idle-timeout signal from
profiler_base.h to alerting.
In `@src/common/platform/include/host/profiler_base.h`:
- Around line 1226-1235: Change the idle-timeout log in the collector monitoring
block from LOG_ERROR to LOG_WARN while preserving its message, arguments, and
one-time detector reset behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 68e19f65-00dc-4365-a095-41ca27267b48
📒 Files selected for processing (5)
docs/dfx/profiling-framework.mdsrc/common/platform/include/host/buffer_pool_manager.hsrc/common/platform/include/host/profiler_base.htests/ut/cpp/common/test_buffer_pool_manager.cpptests/ut/cpp/common/test_profiler_base.cpp
2 participants
Summary
问题发生在 management thread 向 collector 的 Host ready ring 交接处。旧实现遇到满队列时会 retire 尚未收集的 buffer，导致 dep-gen reconciliation 不完整；新实现保留 buffer 所有权，等待 collector 腾出空位后重试，从而保证交接不丢数据。
修改前
flowchart LR A["management: process_entry()"] --> B["push_to_ready()"] B --> C{"Host ready ring 满？"} C -->|否| D["Host ready ring"] D --> E["collector 收集"] E --> F["reconciliation"] F --> G["生成 deps.json"] C -->|是| H["retire buffer"] H --> I["reconciliation 不完整"] I --> J["不生成 deps.json"]修改后
flowchart LR A["management: process_entry()"] --> B["wait_push_to_ready()"] B --> C{"Host ready ring 满？"} C -->|否| D["Host ready ring"] C -->|是| H["保留 buffer 并等待"] H -->|"collector pop 后唤醒并重试"| B D --> E["collector 收集"] E --> F["reconciliation"] F --> G["生成 deps.json"]问题
management thread 从 device ready queue 中 pop 并确认一个 entry 后，
process_entry()持有该 buffer 唯一仍可访问的副本。旧实现使用非阻塞的push_to_ready()；如果 management 到 collector 之间的 Host ready ring 已满，这个 buffer 会在on_buffer_collected()收集之前被 retire。对于 dep-gen，这会导致采集计数无法完成 reconciliation，并在结束阶段阻止
deps.json生成。具体修改
Host ready ring 位于
src/common/platform/include/host/buffer_pool_manager.h：BufferPoolManager::ready_shards_是 management 到 collector 的全部 Host ready shard 数组。ready_shards_[normalize_shard(shard_index)].queue。.queue的类型是ReadyRing，即SpscRing<ReadyBufferInfo, kReadyQueueCapacity>。kReadyQueueCapacity来自DepGenModule::kReadyQueueSize，当前为PLATFORM_DEP_GEN_READYQUEUE_SIZE = 4 * 2 = 8。1. 增加 Host ready ring 满状态查询
位置：
SpscRing::full()，文件buffer_pool_manager.h。head_和tail_；当head - tail >= Capacity时返回满。2. 增加阻塞式 publish 和双向唤醒
位置：
BufferPoolManager::wait_push_to_ready()、try_pop_ready()和wait_pop_ready()，文件buffer_pool_manager.h。push_to_ready()在 ring 满时立即返回false，无法保证已经从 device ready queue 取出的 buffer 最终交给 collector。wait_push_to_ready()。它循环调用shard.queue.push(info)；失败时保留info的所有权，并在shard.cv上等待，直到 ring 状态变化或queue.full()变为 false，然后重试。notify_epoch改为state_epoch。每次成功 push 或 pop 都执行state_epoch++和notify_one()，因此 producer 与 consumer 都能唤醒对方。try_pop_ready()在 collector 成功 pop 后通知等待空间的 producer；wait_pop_ready()统一复用try_pop_ready()，避免遗漏 pop 侧通知。3. 删除
process_entry()的丢弃路径位置：
ProfilerAlgorithms::process_entry()，文件src/common/platform/include/host/profiler_base.h。修改前：
修改后：
try_pop_aicpu_entry()pop 并确认。此时site.info是这个 buffer 唯一仍可到达的所有权路径；retire 会让 collector 永远看不到它。process_entry()才返回。4. collector idle timeout 后继续存活
位置：
ProfilerBase::poll_and_collect_loop()，文件profiler_base.h。break，后续 final drain 可能在满 Host ready ring 上永久等待，因为已经没有 consumer 负责 pop。execution_complete_ == true且最终 ready queue 排空后退出。5. 增加回归测试
WaitPushReadyWakesOnConsumerAndPreservesFifo：填满ready_shards_[0].queue，验证 producer 阻塞；collector pop 后 producer 被唤醒，并验证 FIFO 顺序。ProcessEntryWaitsForReadySpaceInsteadOfRetiringBuffer：验证process_entry()在满 ring 上等待，新 buffer 最终进入 ready ring，没有进入 done queue，也没有被 release。CollectorStaysAliveAfterArmedIdleTimeout：验证 collector 报告 idle timeout 后仍能收集晚到的 buffer。docs/dfx/profiling-framework.md中的所有权说明和 end-to-end 数据流。为什么选择等待
已关闭的 #1594 使用扩大容量的方案，但这只能推迟溢出发生的时机。Host ring 的容量并不是 final flush 突发量的正确性上界。
保留 buffer 所有权并施加 backpressure，可以确保 device-ready 到 host-ready 的交接不会丢失数据；只要 collector 最终能够继续消费，任意有限突发都可以被完整处理。因此，本 PR 取代 #1594 中基于扩大容量的方案。
测试
pre-commit run --from-ref origin/main --to-ref HEAD：通过。ctest --test-dir tests/ut/cpp/build -LE requires_hardware --output-on-failure：rebase 到最新origin/main后 74/74 通过。python -m simpler_setup.build_runtimes --platforms a2a3：在最终无冲突 rebase 前，host_build_graph和tensormap_and_ringbuffer均构建通过。task_20260803_021153_200471219670，设备0,2,4,6，EP=4，TP=4，结果为[RUN] PASS。--enable-l2-swimlane，没有使用--enable-dep-gen 0。rank0..3/d0/deps.json，合计包含 26,440 个 task 和 92,067 条 edge。silent_loss、队列满导致的 retire、record count mismatch、reconciliation failure 或 fatal error。硬件日志没有直接记录是否进入
wait_push_to_ready()；该阻塞分支由 C++ 回归测试直接覆盖。