Repository navigation
fix(sse): estimate tokens from the object and count JSON length without building it (#7847) - #8558
Conversation
|
Verified locally by checking out your branch in an isolated worktree on top of release/v3.8.49 (tip 3070925). Stacking confirmed directly from the commits, not just the PR description:
Given the stacking, the plan is straightforward: land #8555 first, then rebase this branch — |
…ut building it (cheaper-inference#7847) Two changes with one root cause: several hot paths built a full JSON string only to read its .length, and one of them silently changed the answer. 1. CORRECTNESS -- combo's fallback-compression trigger estimateTokens(JSON.stringify(attemptBody)) took the STRING branch of estimateTokens, which is ceil(length / CHARS_PER_TOKEN) over the raw JSON. An inline base64 image is then charged as if every character of the data URL were prose. Measured on a 200 KB inline image: via string (before) 50,039 tokens via object (after) 1,231 tokens a 40x over-count, tripping fallback compression on requests nowhere near the context window. This is the same class cheaper-inference#8368/cheaper-inference#8401 fixed on the request path; the combo call site was missed. Passing the object routes through extractImageTokens, which charges images structurally. Text-only bodies are unaffected -- verified identical, and pinned by a test. 2. ALLOCATION -- jsonLength() Adds an exact serialized-length walker: same O(n) scan, no string. Used by estimateTokens' object branch and by streamReadinessPolicy (which runs on every streaming request and only ever used .length). Exactness matters because every consumer feeds a threshold, so this is property-tested against JSON.stringify over 4000 generated structures covering escaping, lone surrogates, omitted values, non-finite numbers, toJSON, Date, Map, cycles and BigInt. Anything outside the plain-JSON subset falls back to JSON.stringify for THAT SUBTREE only, so an exotic leaf never forces the message history back onto the allocating path. Honest scoping of the memory win: the string was always transient, and V8 collects it efficiently, so this is not 3 MiB of retained heap. Measured allocation churn over 20 calls on a 3.06 MiB body: 3.1 MiB -> 0.5 MiB, about 6x less. The cheaper-inference#8549 benchmark row for this mechanism measures a HELD string and therefore overstates it; the correctness fix above is the larger deliverable here.
362c4bd to
78b559b
Compare
|
Rebased as planned, but without needing #8555 to merge first. #8548 is the extraction design we're going with (agreed with your read — no deliberate reason for the separate module, and Verified on the rebased head:
On your correction: agreed,
|
|
Correction to my comment above: I measured it on pristine detached checkouts with an empty working tree: The gate is in |
1cfbfc0
into
cheaper-inference:release/v3.8.49
|
Thanks @MumuTW — merged into |
…ut building it (cheaper-inference#7847) (cheaper-inference#8558) Two changes with one root cause: several hot paths built a full JSON string only to read its .length, and one of them silently changed the answer. 1. CORRECTNESS -- combo's fallback-compression trigger estimateTokens(JSON.stringify(attemptBody)) took the STRING branch of estimateTokens, which is ceil(length / CHARS_PER_TOKEN) over the raw JSON. An inline base64 image is then charged as if every character of the data URL were prose. Measured on a 200 KB inline image: via string (before) 50,039 tokens via object (after) 1,231 tokens a 40x over-count, tripping fallback compression on requests nowhere near the context window. This is the same class cheaper-inference#8368/cheaper-inference#8401 fixed on the request path; the combo call site was missed. Passing the object routes through extractImageTokens, which charges images structurally. Text-only bodies are unaffected -- verified identical, and pinned by a test. 2. ALLOCATION -- jsonLength() Adds an exact serialized-length walker: same O(n) scan, no string. Used by estimateTokens' object branch and by streamReadinessPolicy (which runs on every streaming request and only ever used .length). Exactness matters because every consumer feeds a threshold, so this is property-tested against JSON.stringify over 4000 generated structures covering escaping, lone surrogates, omitted values, non-finite numbers, toJSON, Date, Map, cycles and BigInt. Anything outside the plain-JSON subset falls back to JSON.stringify for THAT SUBTREE only, so an exotic leaf never forces the message history back onto the allocating path. Honest scoping of the memory win: the string was always transient, and V8 collects it efficiently, so this is not 3 MiB of retained heap. Measured allocation churn over 20 calls on a 3.06 MiB body: 3.1 MiB -> 0.5 MiB, about 6x less. The cheaper-inference#8549 benchmark row for this mechanism measures a HELD string and therefore overstates it; the correctness fix above is the larger deliverable here.
…ut building it (cheaper-inference#7847) (cheaper-inference#8558) Two changes with one root cause: several hot paths built a full JSON string only to read its .length, and one of them silently changed the answer. 1. CORRECTNESS -- combo's fallback-compression trigger estimateTokens(JSON.stringify(attemptBody)) took the STRING branch of estimateTokens, which is ceil(length / CHARS_PER_TOKEN) over the raw JSON. An inline base64 image is then charged as if every character of the data URL were prose. Measured on a 200 KB inline image: via string (before) 50,039 tokens via object (after) 1,231 tokens a 40x over-count, tripping fallback compression on requests nowhere near the context window. This is the same class cheaper-inference#8368/cheaper-inference#8401 fixed on the request path; the combo call site was missed. Passing the object routes through extractImageTokens, which charges images structurally. Text-only bodies are unaffected -- verified identical, and pinned by a test. 2. ALLOCATION -- jsonLength() Adds an exact serialized-length walker: same O(n) scan, no string. Used by estimateTokens' object branch and by streamReadinessPolicy (which runs on every streaming request and only ever used .length). Exactness matters because every consumer feeds a threshold, so this is property-tested against JSON.stringify over 4000 generated structures covering escaping, lone surrogates, omitted values, non-finite numbers, toJSON, Date, Map, cycles and BigInt. Anything outside the plain-JSON subset falls back to JSON.stringify for THAT SUBTREE only, so an exotic leaf never forces the message history back onto the allocating path. Honest scoping of the memory win: the string was always transient, and V8 collects it efficiently, so this is not 3 MiB of retained heap. Measured allocation churn over 20 calls on a 3.06 MiB body: 3.1 MiB -> 0.5 MiB, about 6x less. The cheaper-inference#8549 benchmark row for this mechanism measures a HELD string and therefore overstates it; the correctness fix above is the larger deliverable here.
refactor/combo-error-classifiers, so the diff currently shows two commits — #8555s extraction plus this fix. GitHub will not let a fork PR target another fork branch, so the base isrelease/v3.8.49. Merge #8555 first and this collapses to a single commit. It genuinely depends on that PR — see "Why stacked".Two changes, one root cause: several paths built a full JSON string only to read its
.length, and one of them silently changed the answer.1. Correctness — the combo fallback trigger over-counted inline images by 40x
estimateTokens(JSON.stringify(attemptBody))takes the string branch ofestimateTokens, which isceil(length / CHARS_PER_TOKEN)over the raw JSON. An inline base64 image is then charged as if every character of the data URL were prose. Measured on a 200 KB inline image:That falsely tripped fallback compression on requests nowhere near the context window. It is the same class #8368/#8401 fixed on the request path — this call site was missed, because it stringifies before handing the value over and so never reaches
extractImageTokens.Passing the object routes through the image-aware path. Text-only bodies are unaffected — verified identical and pinned by a test, so this is a no-op for the common request.
2. Allocation —
jsonLength()An exact serialized-length walker: same O(n) scan, no string. Used by
estimateTokenss object branch and bystreamReadinessPolicy(which runs on every streaming request and only ever used.length).Exactness is not optional — every consumer feeds a threshold, and an approximation would silently shift routing and timeout decisions. So it is property-tested against
JSON.stringifyover 4000 generated structures covering escaping, lone surrogates, omitted values (undefined/functions/symbols, which behave differently in objects vs arrays), non-finite numbers,toJSON,Date,Map, cycles andBigInt.Anything outside the plain-JSON subset falls back to
JSON.stringifyfor that subtree only, so an exotic leaf never forces the multi-megabyte message history back onto the allocating path.Honest scoping of the memory win
The string was always transient and V8 collects it efficiently, so this is not 3 MiB of retained heap. Measured allocation churn over 20 calls on a 3.06 MiB body:
About 6x less churn. The correctness fix above is the larger deliverable here — and the #8549 benchmark row for this mechanism measures a held string, so it overstates the cost. Worth correcting when that PR is reviewed.
Why stacked
combo.tsis frozen at 3642 lines and the gate is shrink-only. This PR adds one line there — andlint-stageds Prettier then reflows an unrelated over-long line (+4) duringpre-commit, pushing it to 3645. #8555 takes the file to 3610, which is exactly the headroom this needs. Rebasing was the honest fix; trimming the comment until it fit would not have survived the formatter anyway.Verification
json-size-exactness(property + edge cases)combo-fallback-token-estimate-7847(image over-count guard)typecheck:core,check:cycles,check-file-size,check:any-budget:t11Confirmed red on
upstream/release/v3.8.49with these files untouched, so not from this PR:live repo: no NEW unexported db modules beyond the frozen allowlistandgetTokenLimit: uses GPT-5.5 Codex model context.Not converted
open-sse/utils/stream.ts:694doesBuffer.byteLength(JSON.stringify(body), "utf8")for a payload-size metric — that needs UTF-8 bytes, not characters, so it wants ajsonByteLengthvariant rather than this one. Left for a follow-up rather than silently changing what the metric means.