perf(engine): speed up FormatDouble shortest-representation scan#899
Merged
Conversation
FormatDouble's general (non-integer) case finds the shortest round-tripping decimal by scanning the Str(V:W) precision width upward and taking the first width that parses back exactly. Issue #812 proposed binary-searching the width to cut probes. A sweep of ~70M doubles showed that is incorrect: FPC Str is not correctly rounded at every width, so the round-trip predicate is not monotonic in W, and a binary search emits non-shortest strings (e.g. 9.18742501042000e+222 instead of 9.18742501042e+222) for ~1 in 520k general-case doubles, violating "k as small as possible" in ES2026 Number::toString. Keep the first-hit linear scan (now documented as load-bearing in ADR 0079) and instead cut per-probe overhead: read Str into a fixed ShortString, strip leading spaces in place, and parse with the locale-free Val instead of Trim + TryStrToFloat. Val selects the identical width — verified byte-for-byte against TryStrToFloat over 74.9M doubles with zero divergence — while avoiding the per-iteration heap allocation and the TFormatSettings scan (~2x faster on this path). Output is unchanged: the full Number+JSON test262 sweep under --prod has identical failure sets to the origin/main baseline. toFixed/toExponential/toPrecision use a separate FormatDoubleToPrecision path and are untouched. Also corrects the stale ES2026 §6.1.6.1.13 -> §6.1.6.1.20 Number::toString clause annotation surfaced by the new ADR. Closes #812 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesFormatDouble first-hit precision scan
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Suite TimingTest Runner (interpreted: 11,006 passed; bytecode: 11,006 passed)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Test runner worker shutdown frees thread-local heaps in bulk; that shutdown reclamation is not counted as GC collections or collected objects.
Benchmarks (interpreted: 437; bytecode: 437)
MemoryGC rows aggregate the main thread plus all worker thread-local GCs. Benchmark runner performs explicit between-file collections, so collection and collected-object counts can be much higher than the test runner.
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results437 benchmarks · PR vs same-runner Interpreted: 🟢 44 improved · 🔴 19 regressed · 374 unchanged · avg +0.9% Typical per-run noise (median variance): interpreted ±2.9%, bytecode ±2.2%. Deltas within noise overlap and read as unchanged. arraybuffer.js — Interp: 🟢 2, 12 unch. · avg +0.2% · Bytecode: 🟢 2, 🔴 2, 10 unch. · avg +4.0%
arrays.js — Interp: 🟢 3, 16 unch. · avg +1.1% · Bytecode: 🟢 3, 🔴 1, 15 unch. · avg -0.2%
async-await.js — Interp: 🟢 1, 5 unch. · avg -1.9% · Bytecode: 🟢 1, 🔴 1, 4 unch. · avg +0.6%
async-generators.js — Interp: 2 unch. · avg -0.3% · Bytecode: 2 unch. · avg +0.8%
atomics.js — Interp: 🔴 1, 5 unch. · avg -2.1% · Bytecode: 🔴 2, 4 unch. · avg -4.2%
base64.js — Interp: 🟢 2, 8 unch. · avg +0.7% · Bytecode: 🟢 1, 🔴 1, 8 unch. · avg -0.0%
classes.js — Interp: 🟢 3, 28 unch. · avg +2.0% · Bytecode: 🟢 1, 🔴 3, 27 unch. · avg +0.0%
closures.js — Interp: 🟢 1, 10 unch. · avg +0.7% · Bytecode: 🔴 1, 10 unch. · avg +0.1%
collections.js — Interp: 🟢 1, 🔴 1, 10 unch. · avg +0.7% · Bytecode: 🟢 1, 11 unch. · avg +3.7%
csv.js — Interp: 🟢 1, 12 unch. · avg +4.3% · Bytecode: 🟢 3, 🔴 1, 9 unch. · avg +2.2%
destructuring.js — Interp: 🟢 2, 20 unch. · avg +1.2% · Bytecode: 🟢 1, 🔴 2, 19 unch. · avg -2.1%
fibonacci.js — Interp: 🟢 1, 7 unch. · avg -0.7% · Bytecode: 🔴 1, 7 unch. · avg -3.3%
float16array.js — Interp: 🟢 1, 🔴 3, 28 unch. · avg -0.6% · Bytecode: 🟢 6, 🔴 1, 25 unch. · avg +2.7%
for-in/for-in.js — Interp: 🟢 1, 2 unch. · avg +1.8% · Bytecode: 🔴 1, 2 unch. · avg -0.5%
for-of.js — Interp: 🟢 1, 6 unch. · avg +1.8% · Bytecode: 7 unch. · avg -3.0%
generators.js — Interp: 🟢 1, 3 unch. · avg +1.0% · Bytecode: 🔴 2, 2 unch. · avg -5.1%
intl.js — Interp: 🟢 1, 5 unch. · avg +0.9% · Bytecode: 🔴 4, 2 unch. · avg -6.3%
iterators.js — Interp: 🟢 1, 🔴 5, 36 unch. · avg -2.2% · Bytecode: 🟢 2, 🔴 1, 39 unch. · avg -0.2%
json.js — Interp: 🟢 1, 22 unch. · avg -0.9% · Bytecode: 🟢 5, 🔴 3, 15 unch. · avg +2.2%
jsx.jsx — Interp: 21 unch. · avg +1.1% · Bytecode: 🟢 1, 🔴 1, 19 unch. · avg -3.6%
modules.js — Interp: 9 unch. · avg +3.3% · Bytecode: 🔴 2, 7 unch. · avg -4.6%
numbers.js — Interp: 🟢 1, 🔴 1, 10 unch. · avg +1.7% · Bytecode: 🔴 1, 11 unch. · avg +2.4%
objects.js — Interp: 🟢 1, 6 unch. · avg +3.4% · Bytecode: 🔴 1, 6 unch. · avg -5.8%
promises.js — Interp: 🟢 3, 9 unch. · avg +1.4% · Bytecode: 12 unch. · avg -0.4%
property-access.js — Interp: 5 unch. · avg +1.2% · Bytecode: 🔴 2, 3 unch. · avg -8.0%
regexp.js — Interp: 🟢 1, 10 unch. · avg +0.1% · Bytecode: 🟢 1, 10 unch. · avg +0.8%
strings.js — Interp: 🟢 1, 🔴 3, 15 unch. · avg -1.5% · Bytecode: 🟢 2, 🔴 2, 15 unch. · avg +0.9%
temporal.js — Interp: 6 unch. · avg +0.3% · Bytecode: 6 unch. · avg +3.7%
tsv.js — Interp: 🟢 1, 🔴 1, 7 unch. · avg -5.3% · Bytecode: 🟢 1, 8 unch. · avg +1.2%
typed-arrays.js — Interp: 🟢 2, 🔴 4, 16 unch. · avg -7.8% · Bytecode: 🟢 2, 🔴 2, 18 unch. · avg +0.3%
uint8array-encoding.js — Interp: 🟢 9, 9 unch. · avg +21.1% · Bytecode: 🟢 9, 🔴 1, 8 unch. · avg +19.1%
weak-collections.js — Interp: 🟢 1, 14 unch. · avg +7.0% · Bytecode: 🔴 5, 10 unch. · avg -9.9%
Deterministic profile diffDeterministic profile diff: no significant changes. Measured on ubuntu-latest x64. Each PR run also builds the |
…o measured end-to-end The "~2x" figure was the isolated probe-loop microbenchmark. End-to-end in the engine (bytecode, --prod), a toString-dominated float workload of 2M Number.prototype.toString calls over 15-17-significant-digit doubles runs in 13.4s with the change vs 18.8s on origin/main — ~1.4x faster (-28% execution time); the engine's per-call string allocation and dispatch are a roughly constant overhead around FormatDouble. The earlier engine measurements were invalid because GocciaScript skips unsupported traditional for(;;) loops by default, so the probe arrays were never populated. Add a `toString non-integer (shortest round-trip)` benchmark to benchmarks/ numbers.js (using for...of) so CI tracks this path; the existing toString bench only covered the integer fast path. Correct the speedup claim in ADR 0079 and the code comment accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
test262 Conformance
Areas closest to 100%
Per-test deltas (+1 / -0)Newly passing (1):
Steady-state failures are non-blocking; regressions vs the cached main baseline (lower total pass count, or any PASS → non-PASS transition) fail the conformance gate. Measured on ubuntu-latest x64, bytecode mode. Areas grouped by the first two test262 path components; minimum 25 attempted tests, areas already at 100% excluded. Δ vs main compares against the most recent cached |
coderabbitai
Bot
added
internal
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/built-ins/Number/prototype/toString.js (1)
96-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one non-integer coercion check outside direct
toString().This PR changes the shared non-integer
FormatDoublepath, but the new regressions only pin directtoString()output here. A singleString(0.1 + 0.2)/`${0.1 + 0.2}`assertion would also guard the other call sites called out in the PR summary from drifting later.Possible addition
+ test("String() and template interpolation share the non-integer shortest-round-trip path", () => { + expect(String(0.1 + 0.2)).toBe("0.30000000000000004"); + expect(`${0.1 + 0.2}`).toBe("0.30000000000000004"); + });🤖 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 `@tests/built-ins/Number/prototype/toString.js` around lines 96 - 110, The current Number.prototype.toString tests only cover direct toString output, so add one non-integer coercion assertion that exercises the shared FormatDouble path through String() or template literal coercion. Use an existing computed fractional value in the Number.prototype.toString suite, and keep the new check alongside the other round-tripping cases so it guards the same behavior via a different call site.benchmarks/numbers.js (1)
57-70: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist the sample list out of
run.Rebuilding
xsand recomputing values like0.1 + 0.2andMath.sqrt(2)on every iteration adds setup overhead unrelated totoString(), so this bench will under-measure regressions in the formatting path itself.Possible refactor
+const ToStringNonIntegerSamples = [ + 0.1 + 0.2, + Math.PI, + Math.sqrt(2), + Math.E, + 123.456789012345, + 9.18742501042e222, + 5.7016275775556e-8, +]; + bench("toString non-integer (shortest round-trip)", { run: () => { - const xs = [ - 0.1 + 0.2, - Math.PI, - Math.sqrt(2), - Math.E, - 123.456789012345, - 9.18742501042e222, - 5.7016275775556e-8, - ]; let total = 0; - for (const x of xs) total += x.toString().length; + for (const x of ToStringNonIntegerSamples) total += x.toString().length; return total; }, });🤖 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 `@benchmarks/numbers.js` around lines 57 - 70, The benchmark in numbers.js is doing avoidable setup work inside the bench("toString non-integer (shortest round-trip)") run function by rebuilding xs and recomputing constants like 0.1 + 0.2 and Math.sqrt(2) on every iteration. Hoist the sample list to a shared constant outside the run callback, then have run only iterate over that precomputed list and sum each x.toString().length so the benchmark measures formatting cost more accurately.
🤖 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.
Nitpick comments:
In `@benchmarks/numbers.js`:
- Around line 57-70: The benchmark in numbers.js is doing avoidable setup work
inside the bench("toString non-integer (shortest round-trip)") run function by
rebuilding xs and recomputing constants like 0.1 + 0.2 and Math.sqrt(2) on every
iteration. Hoist the sample list to a shared constant outside the run callback,
then have run only iterate over that precomputed list and sum each
x.toString().length so the benchmark measures formatting cost more accurately.
In `@tests/built-ins/Number/prototype/toString.js`:
- Around line 96-110: The current Number.prototype.toString tests only cover
direct toString output, so add one non-integer coercion assertion that exercises
the shared FormatDouble path through String() or template literal coercion. Use
an existing computed fractional value in the Number.prototype.toString suite,
and keep the new check alongside the other round-tripping cases so it guards the
same behavior via a different call site.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 090841cd-f973-4461-878b-6f12cf1a3128
📒 Files selected for processing (7)
benchmarks/numbers.jsdocs/adr/0079-formatdouble-first-hit-precision-scan.mddocs/adr/README.mddocs/contributing/code-style.mdsource/units/Goccia.Values.Primitives.pastests/built-ins/JSON/stringify.jstests/built-ins/Number/prototype/toString.js
Resolve ADR number collision: origin/main landed ADR 0079 (parser parenthesized-group relexing, #808), so renumber this PR's ADR to 0080-formatdouble-first-hit-precision-scan and update its code-comment references accordingly.
…mark samples Address CodeRabbit review nitpicks on PR #899: - toString.js: add a coercion test asserting String(), template, and string concatenation share the non-integer shortest-round-trip FormatDouble path, not just direct toString(). - numbers.js: hoist the sample list out of the bench run callback so the benchmark measures formatting cost rather than per-iteration array rebuild. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
coderabbitai
Bot
added
documentation
Contributor
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 `@tests/built-ins/Number/prototype/toString.js`:
- Around line 132-136: The coercion test in the Number prototype suite is
missing some value/path combinations, so expand the existing test in the
toString-related spec to assert String, template literal, and concatenation
coercion for the same representative non-integer values. Update the test case
around the current `test(...)` block so both `0.1 + 0.2` and `Math.PI` are each
covered by all three coercion forms, keeping the assertion intent aligned with
the test title.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 715c1156-1d40-4d8b-87fc-95a6759bbf80
📒 Files selected for processing (5)
benchmarks/numbers.jsdocs/adr/0080-formatdouble-first-hit-precision-scan.mddocs/adr/README.mdsource/units/Goccia.Values.Primitives.pastests/built-ins/Number/prototype/toString.js
💤 Files with no reviewable changes (1)
- docs/adr/0080-formatdouble-first-hit-precision-scan.md
✅ Files skipped from review due to trivial changes (1)
- docs/adr/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- benchmarks/numbers.js
- source/units/Goccia.Values.Primitives.pas
…mples Address CodeRabbit review on PR #899: the coercion test claimed all three coercion paths agree but only checked String()/template for 0.1+0.2 and concatenation for Math.PI, so a regression in the untested combinations would pass. Assert String(), template interpolation, and string concatenation for both representative non-integer values. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 28, 2026
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.
Summary
FormatDouble's general (non-integer) case — the shortest round-tripping decimal used byNumber.prototype.toString,String(x), template interpolation, property-key stringification, andJSON.stringifyof floats — without changing its output.Str(V:W)precision width ("same candidates, fewer probes"). A sweep of ~70M doubles (prod-O4) shows FPCStris not correctly rounded at every width, so the "parses back exactly" predicate is not monotonic inW: 14,241 general-case doubles have round-trip "holes". The upward first-hit scan is robust to them; a binary search converges above the true minimum and emits a non-shortest, spec-violating string for ~1 in 520k general-case doubles (e.g.9.18742501042000e+222instead of9.18742501042e+222), violating "k as small as possible" in ES2026 §6.1.6.1.20Number::toString.Str(V:W)into a fixedShortString, strips leading spaces in place (no heap allocation), and parses with the locale-freeValinstead ofTrim+TryStrToFloat.Valselects the identical width — verified byte-for-byte againstTryStrToFloatover 74.9M doubles with zero divergence — while avoiding the per-iteration heap allocation and theTFormatSettingsscan.Performance (measured end-to-end, not just in isolation)
--prod, atoString-dominated workload of 2MNumber.prototype.toStringcalls over 15–17-significant-digit doubles: 13.4s → vs 18.8s onorigin/main= ~1.4× faster (−28% execution time), stable across 3 runs each.FormatDouble, so the end-to-end figure is ~1.4×. It is a real speedup on float stringification — not neutral, not a regression.benchmarks/numbers.js → "toString non-integer (shortest round-trip)"covers this path going forward (the existingtoStringbench only exercised the integer fast path).Constraints / non-goals
Str's per-width rounding entirely; it's a larger spec-exact rewrite, explicitly deferred (this is a watch-tier item).toFixed/toExponential/toPrecisionuse a separateFormatDoubleToPrecisionpath and are untouched.Verification
./format.pas --checkclean.0.1+0.2,1/3,π,√2) and for several non-monotonic "hole" doubles — the exact values a binary search would break.--prod(bytecode, pinned SHAde8e621c):wrapper_infra_failures: 0, and failure sets are identical to theorigin/mainbaseline (Number 282/58, JSON 136/29) → provably zero conformance movement.Val≡TryStrToFloatequivalence proven over 74.9M doubles (0 selected-width mismatches).Docs
§6.1.6.1.13→§6.1.6.1.20Number::toStringclause annotation (source +code-style.md) surfaced by the new ADR.Closes #812
Testing
Number.prototype.toString,JSON.stringify); no value-type semantics changed.toStringbenchmark tobenchmarks/numbers.js; measured ~1.4× faster end-to-end (2M calls, bytecode,--prod) vsorigin/main.