docs(engine-plan): close #7478 — the JSON tape's scan penalty, re-measured at v0.5.1370 - #7654
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe documentation updates JSON tape benchmarks to v0.5.1370, describes stale-measurement failure modes, records additive tape and collector costs, and closes backlog item 7478 with verification evidence. ChangesJSON tape measurement update
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/engine-plan.md`:
- Around line 343-348: Reconcile the benchmark table’s tape-off 1,287 ms
baseline with the PR objective by identifying whether it belongs to field_access
or parse + full scan, then update the corresponding table cell and closure
evidence consistently. Ensure the field_access before-and-after comparison is
explicit and unambiguous.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| | phase | tape on, then → now | tape off, then → now | | ||
| |---|--:|--:| | ||
| | parse only | 210 → **160 ms** | 1220 → 1196 ms | | ||
| | parse + full scan | 3030 → **1233 ms** | 1287 → 1224 ms | | ||
| | parse + stringify | 254 → **171 ms** | 1756 → 1449 ms | | ||
| | field_access | 2981 → **1721 ms** | — → 1480 ms | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reconcile the old field_access tape-off baseline.
The PR objective identifies 1,287 ms as the old tape-off field_access result. This table records — for that cell and assigns 1,287 ms to parse + full scan. State which workload owns 1,287 ms, then align the table and closure evidence. The current mapping makes the field_access before-and-after comparison ambiguous.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/engine-plan.md` around lines 343 - 348, Reconcile the benchmark table’s
tape-off 1,287 ms baseline with the PR objective by identifying whether it
belongs to field_access or parse + full scan, then update the corresponding
table cell and closure evidence consistently. Ensure the field_access
before-and-after comparison is explicit and unambiguous.
f4befc2 to
b903818
Compare
…sured at v0.5.1370 Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Audit — merging as v0.5.1373Closing #7478 with a measurement is the right outcome, and the brief asked for exactly this. A full scan was 2.3× the direct parser and is now 1.01× (1233 vs 1224), with The decomposition is what makes it a close rather than a guess: measuring all four Both corrections to my brief are right and both are mine. The knob is The method note is the part I most want kept. Discarding the first benchmark run when another agent's build landed mid-flight (load 1.86 → 8.57, every σ 50–600) and re-gating on Gates: 22/22 lint, |
b903818 to
143c7b7
Compare
Closes #7478 — with a measurement, not a change.
#7478 is
docs/engine-plan.md's item 2: "json tape: full-array scans pay 2.3×the direct parser (field_access 2981 ms vs bun 223)". Its three-step roadmap
has shipped in full, so the first thing owed to it was a re-run of its own
decomposition, not more code.
Fresh decomposition — v0.5.1370 (
9617779046)Pinned quiet mini,
taskpolicy -t 0 -l 0, 11 interleaved rounds (every armgets sample r before any arm gets r+1, so drift cannot land on one arm),
176/176 samples asserted, every arm's checksum byte-identical to node 26.5.1.
The headline claim is gone. A full scan cost 2.3× the direct parser
(3030 vs 1287); it is now 1.01× (1233 vs 1224).
roundtripdid not pay forit — it improved, 254 → 171 ms.
Why it closes rather than continues
Measuring all four
PERRY_JSON_TAPE×PERRY_GEN_GCcombinations, rather thanthe two-switch
idiomaticarm the issue's floor came from,field_accessnowdecomposes additively:
The tape costs ~200 ms whichever collector runs; the collector costs ~560 ms
whether or not the tape exists. The issue had these as an interaction (scan
σ 214.9 under gen-GC vs 8.8 under mark-sweep for the identical tape) — that
interaction is what #7539 removed, and it is gone.
The ~200 ms is the tape build (parse-only is 160 ms), structural and already
named in #7537: the build is purely additive whenever the whole tree ends up
materialized anyway, and nothing can predict scan-shaped access before the
parse. The remaining ~560 ms is a generational-collector term the tape-off arm
carries identically — GC-campaign work, not tape policy.
What actually fixed it
Method note added to the doc
The "re-measure before scoping" warning goes from three instances to four, and
gains a second failure mode this ticket demonstrates: a stale floor is as
misleading as a stale headline. #7478's acceptance bar was "materially under
the 1350 ms
idiomaticrow" — butidiomaticis a measurement, not aconstant, and it had moved to 938 ms. Coding to the ticket would have chased a
headline that was already fixed and then declared failure against a bar that no
longer existed.
Verification
rustc == 0and 1-min load< 1.5 held across two consecutive checks. The first attempt was discarded:
another agent's build started mid-run (load 1.86 → 8.57, every σ 50–600).
Reported numbers are two independent quiet runs that agree.
no checksum, and asserts the expected sample count — a pass cannot be
confused with "it never ran".
7.5× on parse-only, 8.5× on roundtrip), and
cargo test -p perry-runtime --lib json_tapeis 24/24 including the two cases that assert whichproducer ran via
reparse_materializations().-p perry -p perry-runtime-static -p perry-stdlib-staticwithPERRY_RUNTIME_DIRpinned and the.amtime asserted to have moved;PERRY_NO_AUTO_OPTIMIZE=1on every compile.Docs-only: no version bump (maintainer bumps at merge), no
CHANGELOG.md.https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Summary by CodeRabbit