Skip to content

perf(sidecar): skip usage rewrite parsing for frames without usage - #2481

Open
sudoalok wants to merge 1 commit into
llm-d:mainfrom
sudoalok:perf/sidecar-cached-tokens-guard
Open

perf(sidecar): skip usage rewrite parsing for frames without usage#2481
sudoalok wants to merge 1 commit into
llm-d:mainfrom
sudoalok:perf/sidecar-cached-tokens-guard

Conversation

@sudoalok

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

replaceCachedTokensSSELine unmarshals every sse frame looking for usage, but only the last frame in a stream has usage in it. so a 500 token response does ~500 full json.Unmarshal calls to end up rewriting one field once.

put a bytes.Contains check for "usage" in front of the parse. frames without it already came out unchanged, they just paid for the unmarshal first, so the output is the same. if the model writes the word usage in its own text the check passes and the parse runs and finds nothing, same as now.

numbers from the benchmarks in this PR:

content frame: 3689 ns/op 1816 B/op 40 allocs/op -> 224 ns/op 0 B/op 0 allocs
usage frame: unchanged

added benchmarks for both frame shapes to the existing test file. existing tests pass, race clean, vet and gofmt clean.

the openai streaming parser already does the same strings.Contains(content, "usage") check before unmarshalling, and its the same idea as the guarded log sites in #2470. came across this while looking at #2447 but its unrelated to that discussion, this path already runs today on the nixl connector.

Which issue(s) this PR fixes:

Fixes #

Release note (write NONE if no user-facing change):

NONE

Signed-off-by: Alok Behera <alokbeherak061@gmail.com>
@sudoalok
sudoalok requested review from a team and roytman as code owners August 20, 2026 13:12
@sudoalok
sudoalok requested review from ahg-g and vMaroon August 20, 2026 13:12
@github-actions github-actions Bot added kind/cleanup area/sidecar size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 20, 2026
@@ -215,3 +216,24 @@ var _ = Describe("Cached token usage rewriter", func() {
Expect(recorder.Body.String()).To(ContainSubstring(`"cached_tokens":7`))
})
})

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.

The two new benchmarks exercise the no-usage guard's performance but don't assert correctness. Every existing It case in this file that reaches replaceCachedTokensJSON has usage in its body, and the one case that doesn't ("should preserve non-JSON streamed data lines") uses non-JSON, which isn't valid JSON either.
So there's no test covering the actual hot path this PR optimizes: a valid-JSON streaming delta with no usage field. If the guard's key or quoting were ever wrong, no spec would catch it, only a benchmark number silently changing.

Could you add something like:

It("should preserve streamed content chunks without usage", func() {
    body := []byte(`data: {"choices":[{"delta":{"content":" the"}}]}` + "\n\ndata: [DONE]\n")
    updated := replaceCachedTokens(body, 7)
    Expect(updated).To(Equal(body))
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sidecar kind/cleanup size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants