Repository navigation
perf(wire): remove an intermediate response decoding copy - #285
Conversation
|
Caution Review failedThe pull request is closed. Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review Please review the response decoder's compatibility adapter, particularly numeric status tags, escaped/duplicate/unknown fields, structured errors, preserve_order, generic deserializer behavior and the differential reference tests. The release comparison uses wholly synthetic data; this draft also awaits CI qualification. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
bb8fef9 to
6ebe95f
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb8fef95cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review Current head bdccb79 includes the generic-map key compatibility fix and map-only root deserializer coverage. The reported issue has a differential test reproduced failing before the fix and passing after it. Please review this head; final release measurements on main's serde_json 1.0.154 are in progress. |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review The latest commit preserves the original generic deserializer's identifier and typed Option entry points. Added differential tests that fail before the fix and pass afterward, alongside map-only, unknown-key and byte-key cases. Default wire tests, wire/IPC/CLI consumer tests with preserve_order, and Clippy pass locally. Final paired release measurements are being rerun on this head. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Response decoding currently buffers and clones an intermediate Serde Content tree around the flattened result. Decode the correlation ID inside the status branch to remove that copy. Public types and derived serialization stay unchanged. Private adapters preserve the original map, identifier and typed request-ID entry points, generic key handling and numeric status identifiers.
Only
crates/wire/src/envelope.rs, its private module and tests change. No dependency, framing or consumer changes. Checked #281's remote diff before editing: no overlap in these paths. Base is main01987cccb23e910628e4cee8ee77b3aa7b1d32af; measured head isc6ac0ee55127fb1221e2372c072aafcc838dcfa7.Synthetic measurements
One Linux x86_64 runner, release/fat LTO, serde_json 1.0.154. Actual baseline and head executables use the identical harness; ran base/head/head/base serially, without concurrent compilation. Each run has 12 samples per scenario, 200 decodes per sample for lists and 1,000 for small responses. Rebuilt the wire package between worktrees and verified distinct executable hashes; dependencies share the usual Cargo cache/target.
With
serde_json/preserve_order, reductions for 100 escaped chats were 22.90% / 21.59%, and for 1,000 were 18.99% / 18.08%. All 44 synthetic scenarios had a positive median reduction in both feature configurations and repetitions; this is not a guarantee for other workloads.Separate test-only heap instrumentation, 100 escaped chats: additional peak requested live heap 208,291 → 129,307 B (37.92% lower); retained DTO heap unchanged at 27,456 B; allocation/reallocation calls 819 → 616. Timing uses the normal allocator. Heap excludes fixture/input bytes and is not RSS.
Separate Callgrind production-entrypoint comparison, 100 decodes of the 100-chat escaped fixture: inclusive decode instructions 147,486,068 → 126,268,187 (14.39% lower). Summed wrapper self instructions are 46,000 → 86,000, below 0.1% of its inclusive count: the gain comes from downstream parsing/materialization. Inclusive counts exclude result drop; timing includes it. Instruction counts are not CPU elapsed time.
These measurements exclude socket/framing, startup, stdout and a connected daemon. No claim about whole-client latency, RSS or real-account workloads.
Compatibility and checks
Differential tests compare decoded values, acceptance and re-encoded responses with the previous derived layout: all 33 success variants and structured errors; field permutations, escaped/duplicate/unknown/missing fields, wrong types, numeric status tags, ID limits, detail ordering, invalid UTF-8, truncated/trailing data and recursion-depth boundaries. Generic tests cover numeric/compound/byte keys, byte tag values, map-only roots, identifier restrictions, and typed option/integer hints. Targeted regressions were reproduced failing before their fixes.
Default wire tests pass. Wire/IPC/CLI consumer suites and all-target Clippy pass with all features and preserve_order, including existing interleaved-event IPC coverage. Codex completed review of c6ac0ee without findings; the initial generic-key finding is fixed and its thread resolved. Remote CI/Pages/artifacts remain queued, so qualification is pending and this stays draft. Local daemon tests could not compile because ALSA development headers are absent; CI provisions them.
Reproduce timing:
cargo test -p oxidezap-wire --release --test response_decode_cost -- --ignored --nocapture response_decode_timing. Heap:cargo test -p oxidezap-wire --release --test response_decode_heap -- --ignored --nocapture. Add--features serde_json/preserve_orderbefore--for that configuration. The timing harness also pairs the previous representation and current decoder in one executable; actual branch comparisons above use each executable's production decoder.