codegen: byte-array reads reach the numeric proofs (bench_int_arithmetic 475 → 395 ms) - #9146
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)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughTyped-array and buffer byte reads now participate in numeric analysis. Arithmetic lowering converts out-of-bounds ChangesTyped-array numeric lowering
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR improves numeric byte-array reads and compound-index bounds proofs while preserving out-of-bounds numeric behavior; no actionable merge-blocking risk remains at the current head after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant lower_arithmetic_operand
participant lower_expr
participant NaNCanonicalization
lower_arithmetic_operand->>lower_expr: lower Uint8ArrayGet with numeric index
lower_expr-->>lower_arithmetic_operand: boxed byte value or undefined
lower_arithmetic_operand->>NaNCanonicalization: compare TAG_UNDEFINED_I64
NaNCanonicalization-->>lower_arithmetic_operand: byte value or canonical NaN
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a detailed summary, concrete changes, performance measurements, correctness coverage, test results, and known limitations. It does not use the template headings or include an explicit related-issue declaration and checklist, but the substantive information is mostly complete.
✨ 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 |
4d26663 to
9895d96
Compare
|
Local gate on the Linux box (perrymaster), commit Suites: Lint: The three FAIL steps are the environment, not the change: For the record, the first gate run caught a real one that the truncated output nearly hid — a rustfmt violation (a stray blank line in the appended collector tests). Fixed and re-gated; the run above is the fixed commit. |
…tic 475 -> 395 ms) Two fixes, both found working benchmarks/suite/bench_int_arithmetic.ts. 1. `acc += px[i]` on a Uint8Array took the dynamic add. The HIR lowers a read on a known Uint8Array/Buffer binding to `Uint8ArrayGet`, but the number-by-construction fixpoint's number-or-undefined view rule and the not-BigInt predicate beside it only matched the `IndexGet` spelling, so the accumulator was never admitted and every += went through js_dynamic_string_or_number_add with acc in a rooted shadow slot. Correctness half: with a raw fadd, an out-of-bounds read's NaN-boxed `undefined` survives the add (IEEE keeps the payload) and acc reads back as `undefined` where the spec says NaN. Number-context lowering canonicalizes first -- one compare and a select, never a call; the undefined box is the only non-double this node can yield, so the test is exact. 2. A compound index (`i * 2 + 1`, `y * 30 + x`) could not prove its bounds: bounds_for_buffer_access_width proved only a single index local with a bounded-pair fact, or a constant, so each element paid a js_uint8array_index_get_value call. int_range_expr composes the leaves' range facts with checked arithmetic; when the whole interval fits inside a CONSTANT buffer length the access is in bounds every iteration. A non-constant length still declines. Mac mini, min of 3, self-timed: bench_int_arithmetic 475 -> 395 ms (node 64); a pixels[y * SIZE + x] variant 636 -> 356; a probe whose reads all qualify goes from 54 per-element helper calls to none. Byte-identical to node on two differentials (8 cases around the byte read: in-bounds accumulate, OOB inside +=, OOB as a value, string concat, negative and fractional indices, Buffer receiver, mixed accumulator; 7 around the bounds proof: exact kernel shape, interval touching the last element, counter mutated in the body, out-of-range interval, one-past-the-end, negative composite index, non-constant length). perry-codegen: 10 collector tests and all 284 native_proof_regressions pass. Not fixed here: the bench's own (y + ky) * SIZE + (x + kx) still declines the interval proof. Landmine documented at the call site: f64_kind_from_class maps Uint8Array/Uint8ClampedArray to a kind whose checked load assumes the typed-array header (length at handle+0, elements at handle+16), while perry's new Uint8Array(n) is buffer-backed (length at data-8) -- routing one there makes every index read out of bounds and silently yields undefined. Claude-Session: https://claude.ai/code/session_012Ys25ni6VwDKE71o1NTYAT
9895d96 to
722d3a1
Compare
|
Merged via a merge train — cherry-picked with two other PRs onto one branch and validated together in a single build. Final validation: hir 365 passed, codegen 1356, runtime 2831 passed (exit 0, 0 abort markers), perry --bins 1066, The train initially also carried #9140 (tombstone reuse for small-object churn), which failed four delete/shape-transition tests on their own premise ( |
|
Follow-up pushed ( The leftover I described ( What isolated it: changing only that benchmark's allocation to a literal length, nothing else, took it from 395 → 168 ms with the calls gone. The predicate asks whether the allocation's size is fixed, never what it is — Re-measured against this branch's exact merge-base (
7.5× Node → 2.4×. Checksums identical on both binaries. The three changes only compound together: the fixpoint fix makes the accumulation native, the allocation fix gives the receiver a view, and the interval proof is what lets a compound index use it. Take any one away and the reads go back to calls. Still verified on this base: both differentials byte-identical to Node (eight cases around the byte read, seven around the bounds proof), |
|
Gate re-run on the new tip |
1 participant
Two numeric-lowering fixes for byte-array reads, both found working
benchmarks/suite/bench_int_arithmetic.ts— the worst entry on a fresh perry-vs-node sweep (7.7× Node before this; 6.2× after, with the remaining gap identified below).1.
acc += px[i]on aUint8Arraytook the dynamic addThe HIR lowers a read on a binding it already knows is a
Uint8Array/Bufferto the dedicatedUint8ArrayGetnode. The number-by-construction fixpoint's number-or-undefinedview-read rule — and the not-BigInt predicate beside it — only recognised theIndexGetspelling, so the accumulator was never admitted and every+=lowered throughjs_dynamic_string_or_number_add, withaccpinned in a GC-rooted shadow slot.This fix has a correctness half. Once the add is a raw
fadd, an out-of-bounds read's NaN-boxedundefinedsurvives it — IEEE arithmetic preserves the NaN payload — and the accumulator reads back asundefinedwhere the spec saysNaN. Number-context lowering now canonicalizes the value first: one compare plus a select, never a call. Theundefinedbox is the only non-double this node can produce, so the test is exact.2. Compound buffer indices could not prove their bounds
bounds_for_buffer_access_widthproved only a single index local carrying a bounded-pair fact, or a constant. An index likei * 2 + 1ory * 30 + xtherefore fell back to a per-elementjs_uint8array_index_get_valuecall. The interval analysis can bound those: every leaf is a counter with a range fact or a compile-time constant, andint_range_exprcomposes them with checked arithmetic, answeringNoneas soon as a leaf is unknown or a step overflows. When the whole interval fits inside a constant buffer length, the access is in bounds on every iteration. A non-constant length still declines — there is nothing to compare the interval against.Measurements
Idle Mac mini, min of three, self-timed (the benchmark's own printed elapsed, not wall clock — node's ~76 ms startup otherwise flatters perry):
bench_int_arithmeticpixels[y * SIZE + x]variantCorrectness
Byte-identical to node on two differentials that belong to neither suite:
+=, an out-of-bounds read as a value, string concatenation, negative and fractional indices, aBufferreceiver, and a mixed numeric/string accumulator;perry-codegen: 10 collector tests and all 284native_proof_regressionspass.Two things left on the table, deliberately
(y + ky) * SIZE + (x + kx)still declines the interval proof (54 reads per pixel remain calls), so this is a step, not the finish. It is not the counters' negative start — a non-negative rewrite still declines.f64_kind_from_classmaps"Uint8Array"and"Uint8ClampedArray"to a typed-array kind, but the checked load it feeds reads the length athandle + 0and elements athandle + 16. Perry'snew Uint8Array(n)is buffer-backed (length atdata - 8), so handing that path one of those receivers makes every index compare out of bounds and silently yieldsundefined— I hit exactly that (checksum:0instead of5760000) while writing this, and backed it out. Nothing reaches it today;Int32Array/Float64ArrayandUint8Arrayparameters are unaffected.An earlier attempt to also make the kernel read
K[ky+1][kx+1]a native multiply is not in this PR: the flat-const rodata lowering is not what actually runs for that shape, so claiming the value is a canonical raw double would have been unsound, and the residual coercions it added measured net-negative (475 → 518 ms).https://claude.ai/code/session_012Ys25ni6VwDKE71o1NTYAT
Summary by CodeRabbit
Uint8ArrayandBufferbyte reads, including arithmetic and compound assignments.NaNwhen used in numeric operations.