fix(json): DirectParser decimal→f64 now matches the tape materializer and node (#7477) - #7483
Merged
Merged
Conversation
added 2 commits
August 6, 2026 06:05
…umber (#7477) The small fixed-point fast path computed `int_acc as f64 + (frac_acc as f64 / 10^k)` — two IEEE roundings (division, then addition) — which is one ulp off str::parse::<f64> for literals like 260.75197. The tape materializer uses str::parse, and node (V8 strtod) agrees with the tape, so the DirectParser was the wrong one. Replace it with the Clinger fast path: accumulate every digit into one integer mantissa; when mantissa <= 2^53 (exactly representable) and the scale is an exact power of ten, a single IEEE division of exact operands is correctly rounded — bit-identical to str::parse. Wider tokens fall through to str::parse on the full slice.
📝 WalkthroughWalkthroughThe DirectParser decimal fast path now combines digits into one mantissa and performs one division to avoid double rounding. Broader values use the fallback parser. Regression tests compare bit patterns with Rust ChangesDirect parser number precision
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@changelog.d/7483-directparser-float-parity.md`:
- Line 1: Update the old-result decimal rendering in the changelog entry to
260.75197000000004, keeping the associated hexadecimal value and all other
release-note details unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 56b2b415-3260-48d7-a636-f11fb0c6d865
📒 Files selected for processing (3)
changelog.d/7483-directparser-float-parity.mdcrates/perry-runtime/src/json/mod.rscrates/perry-runtime/src/json/parser.rs
| @@ -0,0 +1 @@ | |||
| **Fixed: JSON DirectParser decimal→f64 now bit-identical to the tape materializer and node (#7477, PR #7483).** The DirectParser's small fixed-point fast path computed `int as f64 + (frac as f64 / 10^k)` — two IEEE roundings — and was one ulp off the correctly-rounded value for literals like `260.75197` (returned `0x40704c0811b1d92c` = 260.75197000000003 instead of `0x40704c0811b1d92b`). The tape materializer uses `str::parse::<f64>` (correctly rounded, matches V8 strtod), so every DirectParser-routed parse — blobs under 1 KB or over 16 MB, all non-array roots, `PERRY_JSON_TAPE=0` — silently got divergent floats, which also surfaced as longer restringified output (`260.75197000000003` round-trips at +9 chars). Fix in `crates/perry-runtime/src/json/parser.rs`: the fast path now accumulates all digits into one integer mantissa and, when the mantissa is ≤ 2^53 and the scale is an exact power of ten, performs a single correctly-rounded IEEE division (the Clinger fast path); wider tokens fall through to `str::parse` on the full token. `bench_field_access` checksum under `PERRY_JSON_TAPE=0` moves from 2552986400 to 2552985550, matching the tape path and node. New unit test `direct_parser_number_bits_match_strtod` pins the diverging literals plus the full `i * 3.14159` value space against `str::parse` bits; all 12 json gap tests remain byte-identical to node v26.5.1; perf on `bench.ts` / `json_parse_1mb` is neutral within host noise. | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the old-result decimal rendering.
Line 1 says 0x40704c0811b1d92c rendered as 260.75197000000003. The parser regression documentation identifies its shortest round-trip output as 260.75197000000004. Use the same value in this release note.
🤖 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 `@changelog.d/7483-directparser-float-parity.md` at line 1, Update the
old-result decimal rendering in the changelog entry to 260.75197000000004,
keeping the associated hexadecimal value and all other release-note details
unchanged.
proggeramlug
added a commit
that referenced
this pull request
Aug 6, 2026
…own doc (#7484) * docs(engine-plan): split into status quo + remaining work; history to its own doc The plan had grown to 811 lines, most of it dated status sections and resolved incident narratives. It now holds only: the current state of the four GC layers, the repsel stack position, the measured performance backlog, the live gates/blockers, the ordered remaining work, and the binding method rules distilled from the incidents. Everything dated moved verbatim to docs/engine-plan-history.md as provenance. Also reflects #7477 fixed by #7483 (merged): the DirectParser float divergence is closed and #7478 is unblocked. * docs: changelog fragment for 7484 --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
pushed a commit
that referenced
this pull request
Aug 6, 2026
`force_materialize_lazy` walked the tape element-by-element, which the #7478 decomposition measured at ~2.3x the direct parser's batch tree build (56 ms/iter vs 24 ms/iter on the 10k-record fixture). A lazy array only ever stands for a top-level array, so its retained blob is exactly that array's source: re-parsing it with `DirectParser` produces the same tree, and since #7483 put the DirectParser's decimal fast path on one correctly-rounded division, the same numbers bit-for-bit. The reparse runs inside a nesting-safe `GcSuppressScope`. `DirectParser` holds `input: &[u8]` derived from the blob for the whole parse, carries an unrooted raw-pointer shape cache, and fills fresh arrays through `note_array_slot_layout_only` (which skips the generational barrier on the strength of that suppression) - all three are only sound in a no-move window, which is why the first attempt SIGSEGV'd. Cached elements are patched back over the fresh slots through `store_array_slot`, so a handed-out (and possibly mutated) element keeps both its value and its identity, and a pointer landing in a RawF64-layout array downgrades the layout instead of hiding from the tracer. Once most elements are already cached the element-wise merge is the cheaper producer, so the reparse only fires below the measured crossover. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH
proggeramlug
added a commit
that referenced
this pull request
Aug 6, 2026
#7478) (#7499) * perf(json): batch-materialize a lazy tape array by re-parsing its blob `force_materialize_lazy` walked the tape element-by-element, which the #7478 decomposition measured at ~2.3x the direct parser's batch tree build (56 ms/iter vs 24 ms/iter on the 10k-record fixture). A lazy array only ever stands for a top-level array, so its retained blob is exactly that array's source: re-parsing it with `DirectParser` produces the same tree, and since #7483 put the DirectParser's decimal fast path on one correctly-rounded division, the same numbers bit-for-bit. The reparse runs inside a nesting-safe `GcSuppressScope`. `DirectParser` holds `input: &[u8]` derived from the blob for the whole parse, carries an unrooted raw-pointer shape cache, and fills fresh arrays through `note_array_slot_layout_only` (which skips the generational barrier on the strength of that suppression) - all three are only sound in a no-move window, which is why the first attempt SIGSEGV'd. Cached elements are patched back over the fresh slots through `store_array_slot`, so a handed-out (and possibly mutated) element keeps both its value and its identity, and a pointer landing in a RawF64-layout array downgrades the layout instead of hiding from the tracer. Once most elements are already cached the element-wise merge is the cheaper producer, so the reparse only fires below the measured crossover. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH * docs(changelog): add #7499 json reparse-on-materialize fragment Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH * test(json): express the reparse mutation test through across_mut The new #7478 test introduced three bare `get_raw_*_ptr` reads, pushing `gc/tests/runtime_roots/callback_scanners.rs` from 47 to 50 and failing `scripts/raw_handle_debt.py` (1002 vs the 999 baseline). Every one of them was a header read carried across an allocating call, which is the shape `RuntimeHandle::across_mut` exists to express: `lazy_get` and the field-set both return the post-collection header now, and the key string is allocated inside the same window with the receiver re-derived after it. The rooted key handle is gone with it - nothing allocates between its creation and its only use. Back to 999 = baseline. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH * test(json): drop the unused intermediate header binding `cargo test -p perry-runtime --lib` warned `unused variable: hdr` — the refreshed header from the `lazy_get` pairing is shadowed by the one from the field-set pairing before anything reads it. Bind it as `_` and say why in a comment, rather than carrying a name that reads as if the pointer were still live. Claude-Session: https://claude.ai/code/session_019EHcmXKArA7m42SihYCcgH * chore: bump version to 0.5.1288 --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7477.
Root cause
DirectParser::parse_number's small fixed-point fast path (crates/perry-runtime/src/json/parser.rs) computedint_acc as f64 + (frac_acc as f64 / 10^k)— two IEEE roundings: the division rounds once, then the addition rounds again. That double rounding is one ulp off the correctly-rounded result for some literals. The tape materializer (json_tape.rs::materialize_number) usesstr::parse::<f64>(), which is correctly rounded and matches V8's strtod — so the DirectParser was the wrong one, exactly as the issue predicted.Minimal diverging literals
From
bench_field_access's value space (i * 3.14159), exactly two literals diverge:str::parse)260.75197(i=83)0x40704c0811b1d92b= 260.751970x40704c0811b1d92c= 260.75197000000003521.50394(i=166)0x40804c0811b1d92b= 521.503940x40804c0811b1d92c= 521.5039400000001The wrong doubles stringify as
260.75197000000003(+9 chars) and521.5039400000001(+8 chars): +17 chars per restringify × 50 iterations = the 850 checksum delta. Isolation probes confirmed it is a pure parse divergence (JSON.parse("260.75197")alone returns the wrong bits) that surfaces through stringify length; the benchmark'snested.xsum (integers) and even the float-value sum are unaffected (the 2-ulp error is absorbed by summation rounding).Fix
Replace the two-step accumulation with the classic Clinger fast path: accumulate all digits into one integer mantissa; when
mantissa <= 2^53(exactly representable as f64) and the scale is an exact power of ten (all powers up to 10^22 are; the table stops at 10^9), a single IEEE division of two exact operands is correctly rounded — bit-identical tostr::parse::<f64>. Anything wider falls through tostr::parseon the full token slice.json_tape.rsuntouched.Verification
json::tests::direct_parser_number_bits_match_strtod(committed red first at 1aa2228): pins the two diverging literals + negations + assorted shapes through everyparse_numberarm + all 10,000 shortest-repri * 3.14159renderings againststr::parsebits. Green post-fix.mantissa * (1.0 / 10^k)turns the test red on260.75197with the exact pre-fix bit pattern; restored.cargo test -p perry-runtime --lib json: 60 passed, 0 failed.cargo test -p perry-runtime --lib: 1713 passed, 0 failed, 3 ignored (= 1716 total).bench_field_accesschecksum (node v26.5.1 ground truth, run locally):2552985550PERRY_JSON_TAPE=0→2552986400(wrong),=1→2552985550PERRY_JSON_TAPE=0→2552985550,=1→2552985550— all three agree.test-files/test_gap_*json*.tscompile, run, and are byte-identical to node v26.5.1 output (exit codes 0/0 on every one).benchmarks/json_polyglot/bench.ts(auto): pre min 223 ms → post min 222 msjson_parse_1mbkernel (auto → DirectParser, object root): round 1 pre 591 / post 618 ms; round 2 pre 761 / post 735 ms — neutral within noise, rounds bracket each otherbench_field_accessPERRY_JSON_TAPE=0: pre min 4176 ms → post min 4132 mscargo fmt --all -- --check: clean.python3 scripts/raw_handle_debt.py: exit 0, 999 = baseline.scripts/check_file_size.sh: OK.Notes
int_len + frac_len > 17digits ormantissa > 2^53now takestr::parseinstead of a hand-rolled (and wrong) result. The benchmark shapes (≤ 11 significant digits) all stay on the fast path.u64 as f64is a single correctly-rounded conversion of an exact integer, same asstr::parseon the digit string.Summary by CodeRabbit