-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(codegen,runtime): canonicalise NaNs read out of an ArrayBuffer — user bytes can currently forge a pointer (#10779) #10785
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
efa2050
1246fd9
8d46937
2b2b890
4c0a29b
2e2fb18
392b635
0a0af40
70ad5cf
1794e99
a51529b
31efa00
ddf54ad
3baab97
a819742
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| **Indexed reads on an ordinary `Array` no longer re-prove a loop-invariant receiver on every element.** | ||
|
|
||
| An indexed read cost **87 instructions per element** — against 6 for the same arithmetic on a `Float64Array` and 16 for node — and none of it was a runtime call. 56 of the 87 were loop-invariant receiver revalidation re-executed every iteration: the NaN-box tag and handle-band test, the forwarding-flag follow, and a six-load live-head guard. | ||
|
|
||
| perry already had tiers that hoist that proof into the loop preheader. They were declining at one gate, `array_static_type_excluded` — a *declared static type* test in front of a tier that is otherwise fully runtime-guarded — so `const a: number[]` got it and plain `new Array(400)`, which infers `Array<any>`, did not. Ordinary JavaScript never reached the tier it already had. | ||
|
|
||
| Separately, `a[i] += 1` cost **948** instructions per element, 3.7× the identical `a[i] = a[i] + 1`, and no annotation helped: the compound-assignment spill temporaries were minted as `Type::Any`, erasing the receiver's array-ness and the index's integer-ness before codegen saw the statement. | ||
|
|
||
| Array read **87 → 13.5** (node 16.3), `a[i] += 1` **948 → 273**, `a[i] += b[i]` **1025 → 347**. A particle simulation over four numeric arrays spends **60.9% fewer instructions** and **59% less peak RSS**. The bare loop and both `Float64Array` paths are unchanged to the instruction. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| **Stores to an ordinary `Array` element no longer re-prove a loop-invariant receiver on every element.** | ||
|
|
||
| An indexed write cost **105 instructions per element** — against 8 for the same store to a `Float64Array` and 12 for node — with zero runtime calls. **51 of the 105 were loop-invariant** receiver revalidation, and a further 42 was a write-barrier decision provable away from the value's type. | ||
|
|
||
| This widens the store admission the way #10731 widened reads. The gate was `has_materialization_hazard`, which a trailing `console.log` is enough to set. | ||
|
|
||
| `a[i] = k + i` **105 → 17.4**, `a[i] = a[i] + 1` **256 → 24.5** (node 18.7), `a[i] = a[i] + b[i]` **333 → 35.9**. The bare loop, both `Float64Array` paths and the indexed read are unchanged to the instruction. | ||
|
|
||
| Note this moves none of the five real programs in #10695 — their loop bodies are multi-statement or contain calls, which no current tier admits (#10741) — and `a[i] += 1` is unaffected because its lowering is two statements (#10743). | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| **`a[i] += 1` reaches the same loop tier as `a[i] = a[i] + 1`.** | ||
|
|
||
| The two spellings are the same operation and node compiles both to the same cost. perry compiled them **11× apart** — 277 instructions per element against 24 — and the slow one was the idiomatic spelling. | ||
|
|
||
| HIR lowers a compound member assignment into two immutable alias `Let`s plus the store, so the base and the key are each evaluated exactly once and before the right-hand side. The classic range-loop matcher admits exactly ONE statement, so the lowering guaranteed the statement could never reach the tier. Annotating the array changed nothing: the obstacle is the statement count, not type information. | ||
|
|
||
| The temporaries stay. They are load-bearing — an RHS call can reassign the bindings they were read from, and the store must still land at the index evaluated before it ran. Instead the matcher folds them, and only for the guarded fast clones: the slow clone lowers the statements as written, so a failed guard and every side exit still execute the specified evaluation order. Inside the matched subset the fold is exact, because the body walk is a whitelist that admits no call, closure, `await`, update or assignment anywhere in the statement — nothing can write the locals the aliases read. | ||
|
|
||
| `a[i] += 1` **277 → 25.5**, `a[i] -= 1` **208 → 27.5**, `a[i] += b[i]` **347 → 35.9** (identical to `a[i] = a[i] + b[i]`), `a[i] *= 1` **206 → 25.5**, `a[i] |= 0` **236 → 52.5**. The bare loop, both `Float64Array` paths, the indexed read and write, and both expanded spellings are unchanged — their emitted LLVM IR is byte-identical. | ||
|
|
||
| This needs none of #10741's mid-iteration side-exit discipline: the folded-away statements perform no stores, so there is nothing to un-do when a guard fails partway. It also moves none of the five real programs in #10695 — their loop bodies are still multi-statement or contain calls, which no current tier admits. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| **A hoisted `const K = "a"` used as a property key no longer costs 7.3× the same read spelled `o.a`.** | ||
|
|
||
| `o["a"]` in source is already folded to `o.a` by the member lowering. But `module_const_fold` substitutes a hoisted const into the key position *after* that matcher has run, so the node stayed an `IndexGet` and was resolved by name at runtime on every read — UTF-8-validating the key, hashing it for the accessor Bloom summary, classifying the receiver and scanning the shape's key array. | ||
|
|
||
| Re-applying the same fold takes `O[K] + O[J]` from **1236 to 169 instructions**, exactly what `O.a + O.b` costs. It also fixes a spec divergence: `null[K]` and `undefined[K]` read `undefined` before, where node throws. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| **Plain numbers no longer take the slow arms of the string-coercion ladders.** | ||
|
|
||
| `js_string_coerce` and `js_jsvalue_to_string_method` reached their plain-number arm last, through a seven-way jump table, though `is_number()` is a single range test and the exact complement of the arms it skips. `js_number_to_string`'s admission check also forced a 14-instruction saturating `f64`→`u64` cast on a value already proven to be within the 256-entry small-integer cache. | ||
|
|
||
| `n.toString()` **−19.3%** (536.3 → 433.0), `String(n)` **−14.2%** (190.0 → 163.0), template literal −4.7%, large integers −3.2%, floats −0.9%, `"" + n` unchanged. No row regresses. | ||
|
|
||
| `n.toString()` was costing `String(n)` **plus exactly 103 instructions** at every value range — a four-frame dispatch detour through a thread-local one-shot — which is what this removes. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| **`Ptr<Shape>` is no longer disabled in entry bodies for a bug that was fixed in the runtime.** | ||
|
|
||
| `RepselContextFlags::derive`'s `Entry` arm forced `allows_ptr_shape: false` because *"#6991 is an open rooting bug in exactly that position"*. #6991 was closed by #7249, which put `populate_global_this_builtins` inside a `GcSuppressScope` — a runtime fix. The gate has been guarding a bug that no longer exists, and a shape proof in an entry body was being made, counted as a win, then dropped at every access site. | ||
|
|
||
| A module-level `const` with its loop at module level goes **110.00 → 88.99 instructions per iteration (−19.1%)**. The same body inside a function is the control and correctly does not move. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| **`toFixed(7)` no longer costs 10.7× `toFixed(6)`.** | ||
|
|
||
| `spec_to_fixed` asked `format!("{x:.1100}")` on every input — 1100 being the smallest subnormal's worst case — so `(6.0).toFixed(7)` expanded 1100 decimal places through dragon4 and discarded 1093. And `POW10` was seven entries local to `fmt_fixed_int` while the admission bound read `dp <= 6` a hundred lines away, as though it were an overflow limit rather than a table length. | ||
|
|
||
| dp 7 goes **7125.4 → 633.1** and dp 8 **7159.1 → 645.6**, turning a 7.5× loss against node and bun into a win. A money-shaped `(12.34).toFixed(8)` goes **12637.2 → 596.6, 21.2×**. dp 0–6 gain 4–5% as well, because `10u64.pow(dp)` becomes a table load. | ||
|
|
||
| The table is now `POW10_FIXED` at module scope with 20 entries, and its length *is* the admission bound rather than a second constant that happens to agree. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| **Unary `+` now proves a Number by construction.** | ||
|
|
||
| `expr_numeric_by_construction` required `rec(operand)` for `Pos` as though it were a soundness guard. It is not: unary `+` is ToNumber, which either completes holding a Number or throws — BigInt and Symbol throw, an object re-enters ToNumber after ToPrimitive, `undefined` is NaN. A throw stores no value, so the store-universe question the fixpoint asks is vacuous there. | ||
|
|
||
| `Neg` and `BitNot` keep their condition, because ToNumeric is BigInt-preserving (`-1n` is `-1n`). | ||
|
|
||
| The missed proof left the *accumulator* unproven, so its add kept a per-iteration tag test: `const v = +o.a; … h += v` goes **20 → 9 instructions per iteration**, the same figure `o.a * 1` and `o.a - 0` already reached. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| **A double read out of an `ArrayBuffer` can no longer forge a boxed value.** | ||
|
|
||
| A bit pattern falling inside the NaN-box tag window read back as its payload integer rather than the NaN it is, and `Number.isNaN` reported `false` on it. This is memory safety rather than arithmetic: 80 of 144 probe patterns diverge on base and **32 SIGSEGV** — `0x7FF9…` reads back with `typeof === "string"`, `0x7FFD…` as `[object Object]`, a pointer forged out of user-controlled bytes. Seeded GC stress survives **0 of 10 seeds** on base and 10 of 10 after. | ||
|
|
||
| Canonicalising float reads out of an `ArrayBuffer` is sufficient, because a field is a conduit rather than a source — perry already enforces the same invariant for `Array<number>` on the store side. The `Float64Array` element read costs **−0.006%**, and no row where perry beats node regresses. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Document the Float32Array regression. Line 5 records only the Float64Array result. The Float32Array inline tier also increases from 41 to 46 instructions. State this known regression in this release note. Based on learnings, keep one coherent release-note entry that includes the shipped performance impact. 🤖 Prompt for AI AgentsSource: Learnings |
||
|
|
||
| NaN payload bits are no longer preserved through a JS number, matching JSC and SpiderMonkey rather than node. Spec-permitted, and unavoidable under any sound design. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale compound-assignment statement.
This fragment says that
a[i] += 1is unaffected. The same release now admits that operation through the compound-assignment alias fold. Remove this statement or describe the final combined behavior.Based on learnings, changelog fragments must describe the final shipped behavior as one coherent release-note entry.
🤖 Prompt for AI Agents
Source: Learnings