[TRTLLM-13215][perf] Skip redundant one-model sampling-param refills - #17544
Conversation
721314f to
8078d0d
Compare
8078d0d to
f20561a
Compare
|
Hi @lori-ren @allisonlim-nv could you help to review this PR, thanks~ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesSpeculative sampling metadata
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tensorrt_llm/_torch/speculative/interface.py`:
- Around line 1105-1144: Remove slot_ids from
_sampling_params_buffers_need_update signatures: use values alone for the
per-request signature and (values, num_tokens) for the expanded-buffer
signature. Keep the existing comparison, recording, and return behavior
unchanged so slot changes no longer trigger sampling-buffer updates.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9244481a-efe2-46c2-a3f8-d930b62e514d
📒 Files selected for processing (1)
tensorrt_llm/_torch/speculative/interface.py
populate_sampling_params_for_one_model rebuilt and re-uploaded every sampling-parameter buffer on every step, even though a request's params are fixed for its lifetime. Both the host expansion (batch_size * (draft_len + 1) list appends) and the six H2D copies sit on the critical path ahead of the forward. Record what the buffers hold and skip the refill when this step reproduces it. The two buffer groups are tracked separately because they depend on different inputs: the per-request buffers are a function of the ordered sampling values, while the expanded per-token buffers also depend on each request's token count. A context request becoming a generation request grows its span from one row to draft_len + 1 and shifts every later request, so it invalidates the expanded buffers while leaving the per-request ones valid. Slot ids are not part of either signature. Every consumer reads these buffers by batch position, so the order of the normalized values already encodes the batch ordering; slot ids index batch_slot_ids, which is copied separately. Including them would only force refills when a slot changes hands between requests that happen to sample identically. top_k_max derives from the per-request values, so it now refreshes exactly when those buffers do rather than on every call. The signatures live in a list that create_cuda_graph_metadata's shallow copy keeps sharing: the graph views and the eager view write the same tensors, so the record of what those tensors hold has to be shared too. Plain fields would let one view skip a fill another view invalidated. Signed-off-by: ZhaoyangWang <zhaoyangw@nvidia.com>
f20561a to
af6f657
Compare
|
/bot run --disable-fail-fast |
1 similar comment
|
/bot run --disable-fail-fast |
|
PR_Github #65554 [ run ] triggered by Bot. Commit: |
|
PR_Github #65554 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #65741 [ run ] triggered by Bot. Commit: |
|
PR_Github #65741 [ run ] completed with state |
…VIDIA#17544) Signed-off-by: ZhaoyangWang <zhaoyangw@nvidia.com>
Description
populate_sampling_params_for_one_modelrebuilt and re-uploaded every sampling-parameter buffer on every step, even though a request's sampling params are fixed for its lifetime. Per step that is a host expansion ofbatch_size * (draft_len + 1)list appends plus six H2D copies, all ahead of the forward on the critical path. This PR records what the buffers hold and skips the refill when the step reproduces it.request_*(lengthbatch) is a function of the ordered sampling values; the expandedtemperatures/top_ks/top_ps(lengthbatch * (draft_len + 1)) additionally depend on each request's token count, which sets their layout. A context request becoming a generation request grows its span from one row todraft_len + 1and shifts every later request, invalidating the expanded buffers while leaving the per-request ones valid; the same applies whenruntime_draft_lenchanges. A single merged signature would refillrequest_*needlessly on every such transition.batch_slot_ids, which is copied separately; including them would only force refills when a slot changes hands between requests that happen to sample identically.top_k_maxderives from the per-request values, so it now refreshes exactly when those buffers do rather than on every call.create_cuda_graph_metadata's shallow copy keeps sharing. The graph views and the eager view write the same tensors, so the record of what those tensors hold has to be shared too — plain fields would give each view its own stale answer and let one skip a fill another view invalidated, silently sampling with another batch's params.An earlier revision of this PR instead moved the expansion to the device, keeping the params in slot-indexed tables and gathering them with
index_select. That cut host time substantially (batch 128: 186 us to 54 us, and flat in the row count rather than linear) but end-to-end throughput did not improve — it regressed slightly, up to -2.24% per-user at batch 16. The host work is submitted asynchronously (pinned +non_blocking=True), so it is already hidden whenever the GPU is the bottleneck, while the three addedindex_selectkernels are real GPU work on the critical path. Skipping the work entirely avoids both.Test Coverage
Validated on H200 NVL (SM 9.0).
(need_update_sampler_param, need_update_expanded_sampler_param): first call(True, True); steady-state repeat(False, False); context to generation, which changes the token count,(False, True); aruntime_draft_lenchange(False, True); a sampling param change(True, True); slot order reshuffled(True, True); batch growth(True, True); explicit invalidate(True, True). A graph copy shares the signature list, and after the graph view fills, the eager view on the same batch reports(False, False).populate_sampling_params_for_one_modeland comparing every buffer plustop_k_maxagainst a metadata object that refills unconditionally. Identical across the initial fill, a steady-state skip, a generation-to-context transition, a return to steady state, and a sampling-param change. The skip path produces bit-identical buffers to always refilling.top_ksentinel (INT32_MAX) is distinguished from0.test_llama_eagle3_rejection_sampling_modespasses (4 cases), coveringuse_cuda_graphxuse_dynamic_treewithtemperature=1.0/top_p=0.9/top_k=50.No measurable throughput change is the expected shape. The skipped work is host-side and submitted asynchronously, so it is already hidden whenever the GPU is the bottleneck; this removes redundant work rather than shortening the critical path.
JIRA: https://jirasw.nvidia.com/browse/TRTLLM-13215
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.