perf(protocols): stop default fields from defeating the serialize-once skip - #2267
perf(protocols): stop default fields from defeating the serialize-once skip#2267slin1237 wants to merge 1 commit into
Conversation
…heir first serialization GenerateRequest always emitted "return_hidden_states":false, which the sglang default strip then removed, so the unmutated-body fast path in serialize_request_body never reused the first serialization for /generate. ChatCompletionRequest (no_stop_trim, ignore_eos, continue_final_message, return_hidden_states, separate_reasoning, stream_reasoning) and CompletionRequest (no_stop_trim, ignore_eos, return_hidden_states) defeated the same skip. Omit these fields at their defaults: an absent field deserializes to the same default, and the strip already removed the omitted values from outbound bodies. InputIds was an untagged enum, so every tokenized request materialized its token arrays twice through serde's private Content buffer. A manual Deserialize probes the first element (number = Single, array = Batch) and parses the rest in place. Serialize is unchanged. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
|
Warning Your free Security trial is over. An organization admin can activate billing to continue. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 17 minutes Limit details: You’ve used all 3 included reviews currently available. Your 71 included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Comment |
github-actions
Bot
added
protocols
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-tested performance optimization. The serde skip_serializing_if annotations correctly match each field's default, the manual InputIds deserializer handles all edge cases correctly, and the integration tests confirm the reuse path fires. No issues found.
1 participant
Description
Problem
#2139 added a skip that reuses the first serialization of an unmutated request body instead of re-encoding it. It never fires: several non-Option fields with constant defaults always serialize (for example
GenerateRequest::return_hidden_states: false), sostrip_default_sglang_fieldsfinds them, marks every body mutated, and the router re-encodes the full body on every dispatch. On 35MB video bodies that is a full second serialization per request on the buffered path.Solution
Skip serializing fields that sit at their strip-default value, so a default request round-trips byte-identical and the reuse skip actually fires:
GenerateRequest:return_hidden_statesChatCompletionRequest:no_stop_trim,ignore_eos,continue_final_message,return_hidden_states(skip when false);separate_reasoning,stream_reasoning(skip when true - the strip removes these at any value, so their mere presence marked chat bodies mutated)CompletionRequest:no_stop_trim,ignore_eos,return_hidden_states(same defect, same code path)InputIdsdeserializer replacing the untagged enum, which removes serde's internal double-buffering of large token arraysskip_special_tokensis deliberately untouched: the strip keeps it, so it never defeats the skip, and its presence is an asserted wire contract.Changes
is_false/is_truehelpers incrates/protocols(chat, completion, generate)InputIdsDeserialize incommon.rsserialize_request_bodypathTest Plan
mutated == false)set_modelmarks mutated unconditionally) - mitigated since fix(router): pre-size dispatch serialization buffers; trim slack before freezing #2264 by borrowed-slice re-encode with a pre-sized buffercargo test -p openai-protocol237 passed;cargo test -p smg --lib1808 passed;api_tests107,routing_tests120,spec_test96 green; clippy--all-targets -- -D warningsclean; fmt silentChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses