Repository navigation
perf(cli): avoid cloning JSON results into an intermediate tree - #274
Conversation
|
@codex review Please review the borrowed JSON success envelope and the separate release comparison. Check parsed JSON compatibility, null fallback, declared key ordering, and the allocation test's safety and measurement limits. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 33 minutes. View limit details
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 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. |
|
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". |
|
Compatibility audit, updated after the workspace feature-unification failure:
|
|
@codex review The follow-up commit fixes regression-test comparisons for workspace-unified serde_json/preserve_order. The production serializer is unchanged. I reproduced both previous test failures locally with --features serde_json/preserve_order, then confirmed the full CLI suite passes both with and without that feature, plus scoped Clippy. Please review the final head and the JSON value/null-fallback assertions. Key ordering is not part of the CLI contract. |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
CLI JSON results currently clone the entire payload into
serde_json::Valuebefore pretty-printing it. Serialize a borrowed success envelope directly. Preserve the parsed JSON, pretty formatting and null fallback on serialization failure. Object key order follows the serializer; the previous intermediate map's ordering also varied with serde_json/preserve_order. The CLI documents keys and values, not key order.Release comparison on one Linux runner without serde_json/preserve_order for the first table, base algorithm copied verbatim from e6e3020 versus the production helper. Two independent runs, 12 alternating paired samples per scenario, 10 serializations per timing sample. Allocation accounting runs separately with counters disabled during timing. Synthetic MessageDto inputs include UTF-8 and escaped characters.
The same release comparison was repeated twice with
--features serde_json/preserve_order, matching workspace feature unification. The production serializer is identical; only the legacy intermediate map implementation changes.This measures formatting only. It excludes DTO construction, IPC, stdout, process startup and the connected daemon. The input DTO is excluded from additional heap. Requested heap is not RSS; CPU time and RSS were not measured. A test allocator wrapper remains installed during timing, with accounting disabled. Long text remains dominated by escaping and output allocation. The final output String still exists.
Validated
cargo test -p oxidezap-cli --all-featuresboth with and without--features serde_json/preserve_order, scoped all-target Clippy with-D warningsincluding preserve_order, formatting and diff checks. The original two byte-comparison assertions failed under workspace feature unification; the follow-up commit compares parsed values and explicitly checksok=trueanddata=null. The production serializer is unchanged by that follow-up. Regression tests compare parsed JSON including empty payloads/escaping/nulls, and check the existing serialization-error fallback.Reproduce:
cargo test -p oxidezap-cli --release --test json_cost -- --ignored --nocapture.Only CLI output and its isolated test changed. No DTO/protocol, daemon, SQL, account selection, read-only, manifests, lockfiles or #265 changes. No new dependencies.
Summary by cubic
CLI JSON output now serializes a borrowed success envelope directly instead of cloning the entire payload into an intermediate
serde_json::Value, cutting formatting time by up to ~87% and reducing allocations by orders of magnitude.preserve_order.json_cost.rscovering timing, allocation counts, and parsed JSON equality (empty payloads, escaping, nulls, and the failure fallback).Written for commit 4ca5453. Summary will update on new commits.