Skip to content

perf(llm): avoid cloning jail metrics - #11227

Merged
jthomson04 merged 1 commit into
mainfrom
perf-agent/8e5cf6d1c1b7
Jul 6, 2026
Merged

perf(llm): avoid cloning jail metrics#11227
jthomson04 merged 1 commit into
mainfrom
perf-agent/8e5cf6d1c1b7

Conversation

@jthomson04

@jthomson04 jthomson04 commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • move the owned LLMMetricAnnotation out of each Nv stream item before adapting it for dynamo-parsers
  • avoid cloning the optional worker-type strings on every jail input chunk while preserving accumulated metrics
  • improve long ambiguous-prefix streams by 3.89% and ordinary text by 1.29% in paired current-main microbenchmarks

Validation

Investigation validation:

  • cargo fmt --all -- --check
  • cargo clippy --locked -p dynamo-llm --no-default-features --lib -- -D warnings
  • cargo test --locked --release -p dynamo-llm --no-default-features --test test_streaming_tool_parsers --test test_reasoning_parser --test parallel_tool_call_integration --test tool_choice --test tool_choice_finish_reasons (99 passed, 1 ignored)
  • 20-sample-per-role ABBA/BAAB benchmark matrix across seven stream shapes; no statistically resolved regression

Publication recheck on unchanged main:

  • cargo fmt --all -- --check
  • cargo test -p dynamo-llm --locked --no-default-features --test test_streaming_tool_parsers (37 passed)

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of streamed chat responses so usage metrics are preserved more efficiently across tool-calling flows.
    • Reduced unnecessary copying during response processing, which may lower overhead in streaming scenarios.

@github-actions github-actions Bot added perf frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` labels Jul 5, 2026
@datadog-official

This comment has been minimized.

@jthomson04
jthomson04 marked this pull request as ready for review July 6, 2026 16:20
@jthomson04
jthomson04 requested a review from a team July 6, 2026 16:20

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
@jthomson04
jthomson04 force-pushed the perf-agent/8e5cf6d1c1b7 branch from 254ce6f to d98b8a5 Compare July 6, 2026 17:23
@copy-pr-bot

copy-pr-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@jthomson04

Copy link
Copy Markdown
Contributor Author

/ok to test

@copy-pr-bot

copy-pr-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown

/ok to test

@jthomson04, there was an error processing your request: E1

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/

@jthomson04

Copy link
Copy Markdown
Contributor Author

/ok to test d98b8a5

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 36b9e0e9-7710-49fc-9992-b6bf821667d3

📥 Commits

Reviewing files that changed from the base of the PR and between d245a5b and d98b8a5.

📒 Files selected for processing (1)
  • lib/llm/src/preprocessor.rs

Walkthrough

The change modifies apply_tool_calling_jail in the LLM preprocessor to move llm_metrics out of streamed response data using take() instead of cloning it, while preserving existing buffering and re-attachment logic in PendingMetrics.

Changes

Tool-calling jail metrics handling

Layer / File(s) Summary
Move instead of clone llm_metrics
lib/llm/src/preprocessor.rs
The jail_input mapping closure now takes llm_metrics via .take() on a mutable reference instead of cloning it, requiring the closure to bind mut a.

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

Related PRs: None specified.

Suggested labels: None specified.

Suggested reviewers: None specified.

🤖 A tiny hop, a metric set free,
No more cloning, just take() and flee,
One line moved with care,
Through the jail's buffered air,
A small tweak for efficiency.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description omits required template sections, especially Details, Where should reviewer start?, and Related Issues. Add the missing template sections, include a reviewer start file/path, and either link the related issue or confirm none.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: reducing cloning in jail metric handling for llm performance.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

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

@jthomson04
jthomson04 enabled auto-merge (squash) July 6, 2026 17:35
@jthomson04
jthomson04 merged commit d6591c2 into main Jul 6, 2026
100 checks passed
@jthomson04
jthomson04 deleted the perf-agent/8e5cf6d1c1b7 branch July 6, 2026 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` perf size/XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants