[https://nvbugs/6448152][perf] make C++ context-transfer consensus asynchronous - #16634
Conversation
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
|
/bot run --disable-fail-fast --stage-list "GB300-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-1" |
1 similar comment
|
/bot run --disable-fail-fast --stage-list "GB300-12_GPUs-3_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE2-GPU8-Post-Merge-1" |
|
PR_Github #60750 [ run ] triggered by Bot. Commit: |
|
PR_Github #60750 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61052 [ run ] triggered by Bot. Commit: |
📝 WalkthroughWalkthroughChangesThe PR adds an MPI-based Context-transfer consensus
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CacheTransceiver
participant ContextTransferCoordinator
participant PeerRanks
participant MPI
CacheTransceiver->>ContextTransferCoordinator: publish local outcome or timeout
ContextTransferCoordinator->>MPI: send asynchronous event
MPI->>PeerRanks: deliver vote event
PeerRanks->>MPI: send coordinator update
MPI->>ContextTransferCoordinator: receive votes and updates
ContextTransferCoordinator-->>CacheTransceiver: poll consensus result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/tensorrt_llm/batch_manager/cacheTransceiver.cpp (1)
1025-1036: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBlocking consensus loop is a pure busy-wait.
In
blockAllmode this spins onpoll()+std::this_thread::yield()until every awaiting request commits. Local transfers are already drained (the terminal loop callsfuture.get()), so this loop only waits for MPI vote propagation — but if a peer is slow to finish its own transfers, this rank pins a core at ~100% for the entire wait. Consider a short backoff sleep (e.g.kTransferFuturePollIntervalMs) instead of a tight yield.♻️ Suggested backoff
if (blockAll && consensusOutcome.completedRequestIds.size() + consensusOutcome.failedRequestIds.size() < mSenderRequestsAwaitingConsensus.size()) { - std::this_thread::yield(); + std::this_thread::sleep_for(std::chrono::milliseconds(kTransferFuturePollIntervalMs)); }🤖 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 `@cpp/tensorrt_llm/batch_manager/cacheTransceiver.cpp` around lines 1025 - 1036, Replace the tight std::this_thread::yield() polling in the blockAll consensus loop around mergeCoordinatorOutcome() with a short sleep using the existing kTransferFuturePollIntervalMs interval. Preserve the loop’s completion condition and continue merging outcomes before each wait.
🤖 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 `@cpp/tensorrt_llm/batch_manager/cacheTransceiver.cpp`:
- Around line 1025-1036: Replace the tight std::this_thread::yield() polling in
the blockAll consensus loop around mergeCoordinatorOutcome() with a short sleep
using the existing kTransferFuturePollIntervalMs interval. Preserve the loop’s
completion condition and continue merging outcomes before each wait.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1d8480c1-a9f6-4c54-8a78-aa332505d6c8
📒 Files selected for processing (11)
cpp/include/tensorrt_llm/batch_manager/cacheTransceiver.hcpp/include/tensorrt_llm/batch_manager/contextTransferCoordinator.hcpp/include/tensorrt_llm/runtime/utils/mpiTags.hcpp/include/tensorrt_llm/runtime/utils/mpiUtils.hcpp/tensorrt_llm/batch_manager/CMakeLists.txtcpp/tensorrt_llm/batch_manager/cacheTransceiver.cppcpp/tensorrt_llm/batch_manager/contextTransferCoordinator.cppcpp/tests/unit_tests/batch_manager/CMakeLists.txtcpp/tests/unit_tests/batch_manager/contextTransferCoordinatorTest.cppcpp/tests/unit_tests/multi_gpu/cacheTransceiverTest.cppcpp/tests/unit_tests/multi_gpu/mpiUtilsTest.cpp
|
PR_Github #61052 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61120 [ run ] triggered by Bot. Commit: |
|
PR_Github #61120 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #61202 [ run ] triggered by Bot. Commit: |
|
PR_Github #61202 [ run ] completed with state
|
|
/bot run --disable-fail-fast --stage-list "DGX_B200-8_GPUs-PyTorch-3" |
|
PR_Github #61310 [ run ] triggered by Bot. Commit: |
|
PR_Github #61310 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #61345 [ run ] triggered by Bot. Commit: |
|
PR_Github #61345 [ run ] completed with state |
…ynchronous (NVIDIA#16634) Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Summary
Replace the scheduler-hot-path blocking pipeline-parallel terminal-outcome collective in the C++ context cache transceiver with asynchronous ordered agreement.
For the qualified topology, every context pipeline rank publishes one immutable local terminal vote without waiting. The final pipeline rank reduces those votes and publishes one authoritative ordered commit. Request state transitions, retained bookkeeping, and global resource reclamation remain gated on that commit, preserving cross-rank consistency while removing the blocking collective from a naturally staggered scheduler path.
Important
The implementation is functionally validated on this pull request's current head, but current-head token throughput is not fully recovered. The exact-head GB300 run completed 512/512 requests at 794.61 output tokens/s. The 1548.84 output tokens/s result below is a matched result from the earlier frozen validation tree and must not be presented as current-head performance.
Motivation
The blocking context-side pipeline-parallel collective introduced by the original transfer-state-consensus change—not the cross-rank consistency requirement itself—caused the original PP4 throughput regression. That change fixed a real correctness issue: pipeline ranks could otherwise apply different terminal KV-transfer outcomes. Its synchronous execution shape, however, placed the collective at a staggered pipeline scheduler point and converted rank-entry skew into idle time.
The causal comparison used adjacent frozen trees with the same sender wake-up fix and CI compatibility change on both sides. The only product-tree difference between the matched control and treatment was the original two-file consistency patch; its stable patch ID (
df1d9fe1964957dd0a50112a3e46c820a01b7f4f) exactly matched the merged change. The post-change treatment timed out, while transition-only tracing localized thousands of accumulated seconds of asymmetric waiting to the context pipeline consensus rendezvous.The asynchronous protocol retains global agreement and ordered reclamation without forcing every rank to enter the terminal-outcome operation simultaneously.
Protocol and safety invariants
COMPLETEDis committed only when every participant votes completed.Qualified scope and fallback
The asynchronous integration is currently qualified only for the context/sender-side C++ cache transceiver:
All other configurations retain the existing reducer and state-transition path. This is an explicitly qualified fast path, not a correctness-semantics change for unqualified topologies.
The generation-side status path is not optimized here. Eligible workers construct the coordinator symmetrically at startup, but the generation status path does not publish to or poll it; there is no per-request generation-side coordinator traffic. Extending this to TP, CP, attention-DP, or generation-side domains requires explicit request-owner membership and hierarchical shard-to-stage-to-pipeline agreement. Merely relaxing the TP1/CP1 guard could allow different lanes for the same request to commit different outcomes.
Verification
Targeted performance validation uses the exact GB300 PP4 disaggregated workload:
disagg_upload-e2e-gb300_deepseek-r1-fp4_128k8k_con256_ctx1_pp4_gen1_dep8_eplb0_mtp1_ccb-NIXLHistorical causal A/B and repair evidence
The lifecycle trace measured negligible TP consensus time but approximately 4240, 3204, and 2126 accumulated seconds of PP waiting on context ranks 0, 1, and 2, versus 0.082 seconds on the last pipeline rank. Removing only context-side global consensus recovered throughput, establishing the causal phase, but was unsafe. The asynchronous run then recovered matched-control throughput while keeping coordinator mode active on all four context pipeline ranks. Every rank published a local vote and observed the same authoritative global commit; recovery was not obtained by bypassing agreement or globally gated resource retention.
Together, these runs verify both claims: the blocking rendezvous introduced by the original consistency change caused a valid performance regression, and asynchronous ordered commit fixes that regression on the controlled tree without reverting its correctness guarantee.
Current review head
The same targeted workload was run on exact head
a2fca7808c94f90ca396fb047fd3d0887e293706in the current-head GB300 pipeline:Therefore, this head validates protocol correctness and lifecycle safety, but it does not demonstrate recovery of current-head absolute throughput.
Current-head regression isolation
Separate TEST ONLY diagnostics preserve the production coordinator while changing one factor at a time:
The evidence currently places the unresolved slowdown upstream of transfer and asynchronous consensus, in the context pipeline forward/device path or its runtime stack. That investigation remains separate from this production change.
Why this change is still worth merging
The lower current-head number does not invalidate the historical causal result:
This change therefore fixes a real, independently verified issue and is worth merging after normal review and full-CI gates. The merge claim is deliberately scoped: it fixes the consensus-rendezvous regression, but it does not claim to restore the current branch to approximately 1558 output tokens/s. The additional current-head factor remains under investigation.
Additional coverage
Full CI on the official review head is in progress through the current full-CI helper. This pull request should remain draft until required review and full-CI gates are complete.
Follow-up qualification
Future topology work should use hierarchical agreement:
Each extension must cover success, failure, cancellation, timeout, duplicate and out-of-order messages, request-ID reuse with epochs, clean shutdown/drain, and a matched throughput comparison. Independent DP replicas must remain separate agreement domains unless a request explicitly spans them.
Summary by CodeRabbit
New Features
Bug Fixes
Tests