perf(logging): serialize safe_dumps in a single pass on the hot path - #31711
perf(logging): serialize safe_dumps in a single pass on the hot path#31711yassin-berriai wants to merge 1 commit into
Conversation
safe_dumps built a full recursive copy of the payload to strip NUL bytes and detect cycles, then json.dumps re-traversed it; two passes over every logged object. The standard logging payload is serialized on every request, and the per-token cost and cache breakdown added in 1.91 grew that payload, so this was the largest single contributor to the residual proxy overhead versus v1.85.0. Encode the common case (default depth budget, no NUL byte, no circular reference) in a single json.dumps pass, falling back to the existing recursive sanitizer when the output holds a NUL escape, a cycle is hit, or a key json cannot encode. A caller-tightened max_depth always uses the sanitizer so its truncation stays exact. Extracting the sanitizer to module scope also stops it being rebuilt as a closure on every call. About 2.4x faster on a representative 1.9KB logging payload, output unchanged.
|
|
Greptile SummaryThis PR optimizes
Confidence Score: 5/5Safe to merge; all existing contracts are preserved and the documented behavior change for non-string primitive dict keys is intentional and tested. The fast path correctly falls back to the sanitizer for every meaningful edge case (NUL bytes, circular references, un-encodable keys). The only inconsistency found — frozenset serialized as a sorted list by the fast path but as a string by the sanitizer fallback — is an untested edge case that cannot be triggered in isolation and is inconsequential for real logging payloads. All existing tests pass, the fuzz test covers 600 mixed-type payloads, and the performance improvement is well-evidenced. No files require special attention beyond the noted frozenset inconsistency in safe_json_dumps.py.
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/safe_json_dumps.py | Rewrites safe_dumps to attempt a single json.dumps pass and fall back to the recursive sanitizer only on NUL detection, circular reference, or encoding error. The inner _serialize closure is lifted to a module-level _sanitize function. Logic is sound for the common case; minor frozenset divergence between _json_default and _sanitize exists but is low-impact in practice. |
| tests/test_litellm/litellm_core_utils/test_safe_json_dumps.py | Adds a 600-iteration fuzz test comparing the new fast path against a verbatim copy of the previous sanitizer, plus explicit tests for tightened max_depth and non-string primitive keys. Existing tests for circular references, null bytes, pydantic models, and unserializable objects are preserved unchanged. |
Reviews (2): Last reviewed commit: "perf(logging): serialize safe_dumps in a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if max_depth >= DEFAULT_MAX_RECURSE_DEPTH: | ||
| try: | ||
| encoded = json.dumps(data, default=_json_default) | ||
| except (ValueError, TypeError, RecursionError): | ||
| pass | ||
| else: | ||
| # Fall back to string conversion for non-serializable objects. | ||
| try: | ||
| return strip_null_bytes(str(obj)) | ||
| except Exception: | ||
| return "Unserializable Object" | ||
|
|
||
| safe_data = _serialize(data, set(), 0) | ||
| return json.dumps(safe_data, default=str) | ||
| if _NUL_ESCAPE not in encoded: | ||
| return encoded |
There was a problem hiding this comment.
The fast path ignores the default max_depth unless json.dumps itself raises. For an acyclic payload deeper than DEFAULT_MAX_RECURSE_DEPTH but below the JSON encoder recursion limit, safe_dumps(data) now returns the full object instead of inserting "MaxDepthExceeded". The new test only covers caller-tightened depths, so the default depth contract is broken for deep logging payloads.
Artifacts
- Contains supporting evidence from the run (text/x-python; charset=utf-8).
Repro: failing runtime output showing full deep serialization without MaxDepthExceeded
- Keeps the command output available without making the summary code-heavy.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Closing this. Greptile's P1 is correct, and CI's |
|
Thanks for the thorough follow-up and benchmarking — that's exactly the right call. The depth pre-scan turning a net win into a net loss on pydantic-heavy payloads is a meaningful finding worth documenting here for anyone who revisits this later. Closing is the right move given the correctness constraint and the benchmark outcome. |
2 participants
Relevant issues
Linear ticket
LIT-4105
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Follow-up to the OTel per-request import fix (#31707). After that fix, the remaining proxy-overhead gap versus v1.85.0 was profiled with single-worker cProfile, bucketing self-time per request by module. The dependency bumps are net neutral (starlette 1.x is actually faster), so the residual is litellm's own new per-request work, and the largest single slice is
safe_json_dumpssafe_dumpsserialized in two passes: a full recursive Python-level copy of the payload to strip NUL bytes and detect circular references, thenjson.dumps, which re-traverses the whole structure. The standard logging payload is serialized on every request, and the per-token cost and cache breakdown added in 1.91 grew that payload, so the redundant pass became the biggest single contributor to the residual overheadThis encodes the common case in a single pass and falls back to the existing sanitizer only when needed
Microbenchmark on a representative ~1.9KB standard-logging payload, 200k iterations, output asserted identical between old and new
Residual-gap attribution (single-worker cProfile, self-time ms/req, 1.85.0 -> patched 1.91), showing
safe_json_dumpsas the top litellm contributor and starlette as a net winFull benchmark and attribution: https://gist.github.com/yassin-berriai/31ecf15a2ba6d4298c3f2924c734f1a3
This is one increment. The rest of the residual is genuinely new per-request feature work (lazy-feature route matching, agentic-loop gating, callback-capability checks) plus the pydantic validation it pulls in, with no further single hotspot, so the end-to-end throughput contribution of this PR alone is small and below load-test noise; the per-call serialization win is large and deterministic
Type
🚄 Infrastructure
Changes
safe_dumpsnow tries a singlejson.dumpspass with adefaultthat mirrors the sanitizer's handling of pydantic models (model_dump), sets (sorted), and unencodable objects (string fallback). It falls back to the recursive sanitizer when the encoded output contains a NUL escape, a circular reference is hit, or a key json cannot encode. A caller-tightenedmax_depthalways uses the sanitizer, so its truncation stays exact. The sanitizer is lifted from a per-call nested closure to a module-level_sanitize, which also avoids rebuilding it on every callBehavior is locked by a 600-case fuzz test that compares the new path against a verbatim copy of the previous sanitizer across nested dicts, lists, tuples, sets, pydantic models, NUL bytes, and the literal backslash-u-0000 text edge case, plus the existing suite (max_depth truncation, NUL stripping, cycles, non-string keys, pydantic models). Dicts with keys json can stringify (int, bool, None) are now kept with their JSON key form instead of being silently dropped; keys json cannot encode still fall back and are dropped as before