Skip to content

perf(trtllm): isolate sidecar request shutdown waiters - #16082

Merged
jthomson04 merged 1 commit into
mainfrom
jthomson04/trtllm-sidecar-child-cancellation
Oct 12, 2026
Merged

jthomson04 merged 1 commit into
mainfrom
jthomson04/trtllm-sidecar-child-cancellation

Conversation

@jthomson04

@jthomson04 jthomson04 commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Each TensorRT-LLM sidecar generation request now gets a child of the engine shutdown token, self.cancel.child_token(). This matches the vLLM (#14726) and SGLang (#14727) sidecars.

The problem.

  • Today every request clones the engine-wide shutdown token.
  • The response stream's biased select! re-polls shutdown.cancelled() for every gRPC message.
  • Tokio's Notify::poll_notified takes that one token's waiter mutex on each poll, so every in-flight stream contends on a single lock once per streamed token.

Why behaviour is unchanged. A child token is cancelled when its parent is, so cleanup() still ends in-flight streams. Request cancellation is untouched.

Measured A/B. Setup:

  • Tyche, AgentX trace, three nodes: frontend / sidecar with mock TRT-LLM OpenEngine server / Rust aiperf client.
  • The sidecar runs on 36 Grace cores; the 120 s measured windows had 0 failed requests.
4 mock servers, C=4096 Output tok/s req/s Mean ITL Sidecar CPU in cancellation waits
self.cancel.clone() 851K 937 2.24 ms up to 45%
self.cancel.child_token() 1.73M 1,754 0.56 ms 1.2–1.6%

The stock ceiling. With clone() the sidecar topped out at about 0.93–0.97M tok/s and 950–980 req/s, however much concurrency or mock capacity was added. In that state:

  • 24–35 of its 37 tokio workers sat in futex_do_wait;
  • parking_lot lock slow paths under WaitForCancellationFuture took 18–45% of sidecar CPU;
  • request-plane bytes queued unread in the socket.

Measurement notes. One mock TRT-LLM server caps at about 0.8–1M tok/s, so the A/B used four mock processes sharing the gRPC port through a benchmark-only SO_REUSEPORT patch to the mock. That patch is not part of this PR. The numbers were measured at b93847f8ef; this PR applies the same one-line change to current main.

Validation

  • cargo test --locked -p dynamo-trtllm-sidecar --lib: 51 passed, 2 ignored. This includes:
    • cleanup_terminates_an_in_flight_request and a_cancelled_decode_dispatch_is_not_abandoned_mid_flight (shutdown still terminates in-flight streams and releases a pending dispatch);
    • the existing request-cancellation tests.
  • cargo fmt -p dynamo-trtllm-sidecar -- --check and git diff --check passed.
  • The Tyche A/B above. No new tests and no client-visible streaming changes.

Related Issues

  • Confirmed — no related issue.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Generation now responds to engine shutdown through a cancellation signal linked to the engine’s lifecycle. This helps ensure that in-progress generation is notified when shutdown begins, supporting more predictable shutdown behavior.

Give each generation request a child of the engine shutdown token so
concurrent streams stop contending on the shared token's waiter mutex
on every streamed message, matching the vLLM and SGLang sidecars.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
@jthomson04
jthomson04 requested a review from a team as a code owner October 10, 2026 03:02
@github-actions github-actions Bot added the perf label Oct 10, 2026
@coderabbitai

coderabbitai Bot commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 4bd6048d-7dae-4d6a-92e0-70af43bf27a5

📥 Commits

Reviewing files that changed from the base of the PR and between 3d94809 and 9729a36.


📒 Files selected for processing (1)
  • lib/sidecar/trtllm/src/engine.rs

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.



Walkthrough

TrtllmSidecarEngine::generate now observes shutdown through a child cancellation token. The child remains linked to cancellation of the engine’s shared token.

Changes

Shutdown cancellation

Layer / File(s) Summary
Use a child cancellation token
lib/sidecar/trtllm/src/engine.rs
generate creates a child token instead of cloning the shared cancellation token. The child remains linked to parent shutdown cancellation.

Priority: ➖ Normal

Estimated code review effort: 1 (Trivial) | ~3 minutes

Merge Risk: ⚪ Minimal · up to 9729a

Engine shutdown still cancels in-flight streams, and request cancellation behavior is unchanged. No actionable merge risk remains.

Pre-merge checks | Passed 3 | Failed 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check Warning The description provides detailed context, implementation details, performance data, validation results, and reviewer guidance through specific files and tests. However, the required issue reference i… Add a valid issue reference under Related Issues, such as Closes #1234, Relates to #1234, or Closes DYN-1234. Also align the description with the repository template by adding Overview, Details, and Where should the reviewer start sec…
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check Passed The title clearly identifies the TensorRT-LLM sidecar performance change: isolating request shutdown waiters.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The description provides detailed context, implementation details, performance data, validation results, and reviewer guidance through specific files and tests. However, the required issue reference is missing; the description states that there is no related issue instead of linking an issue or Linear ticket.

Resolution

Add a valid issue reference under Related Issues, such as Closes #1234, Relates to #1234, or Closes DYN-1234. Also align the description with the repository template by adding Overview, Details, and Where should the reviewer start sections, if required by repository validation before October 21, 2026, when the issue-link check becomes mandatory. This PR is currently before that date, but the template marks the issue reference as required already.


  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

✅ Dynamo PR CI passed — run 38019186166 (attempt 1) on 9729a36d96

Gate checks: ✅ backend-status-check · ✅ deploy-status-check · ✅ dynamo-status-check

Posted automatically by Devin for run 38019186166. Updated on every full-CI run of this PR.

@jthomson04
jthomson04 merged commit 27541b1 into main Oct 12, 2026
112 of 114 checks passed
@jthomson04
jthomson04 deleted the jthomson04/trtllm-sidecar-child-cancellation branch October 12, 2026 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants