Skip to content

Ask the array again when join meets a hole - #3003

Merged
lahma merged 1 commit into
sebastienros:mainfrom
lahma:fix/array-join-hole-reread
Aug 13, 2026
Merged

lahma merged 1 commit into
sebastienros:mainfrom
lahma:fix/array-join-hole-reread

Conversation

@lahma

@lahma lahma commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

ArrayHoleTraversalBenchmark.JoinHoley is the one row that can move; it runs in the release measurement block with JoinDense and the class's other rows as controls.

[0, funky, , ,].join("") where funky.toString() sets Object.prototype[3] answered "0funky" — node and the spec (Get(O, ToString(k)) per element, every iteration) answer "0funkyfnord". ArrayReadOperations.Get, the read-only fast lane, snapshotted _dense at construction and turned a hole into undefined without ever asking the array.

A null slot (or an index past the snapshot) now routes to JsArray.Get(uint), which still short-circuits to Undefined while CanUseFastAccess holds and walks the chain only after the realm's array prototype chain has actually gained an index property. A slot the snapshot holds is untouched: one bounds compare, one load, one null test — identical to before. The red matrix needed one engine per case to expose, because the first prototype index write sets a sticky realm-wide flag that clears fast access — that mechanism is exactly why the pristine-realm fast path is safe.

Siblings disclosed, deliberately not fixed here: toLocaleString reads via TryGetValue and still short-circuits (spec says Get); ArrayReadOperations.HasProperty answers existence from the same snapshot, so forEach/every/some/filter/indexOf miss an index the prototype gained mid-iteration; and a side effect that reallocates _dense leaves the snapshot itself stale — fixing that one means a per-element field load on the hot path and needs its own benchmark gate.

12 new tests, red on exactly the 4 join/toString rows, both TFMs. test262 standalone 0 / 99,744 / 157, identical.

🤖 Generated with Claude Code

@lahma
lahma force-pushed the fix/array-join-hole-reread branch from 995edf0 to 399e5cd Compare August 12, 2026 06:49
join reads each element with Get(O, ToString(k)) on its own iteration, so a
side effect from one element's ToString is visible to every later one:

    var funky = { toString() { Object.prototype[3] = "fnord"; return "funky"; } };
    [0, funky, , ,].join("");   // was "0funky", node "0funkyfnord"

The read-only array lane snapshots the backing store and answered a null slot
as undefined without ever asking the array. A hole is the absence of an own
element, not the value undefined, so it now resolves through JsArray.Get(uint)
- the same read JsArrayOperations performs, which still answers undefined
without walking anything while CanUseFastAccess holds. Array.prototype.toString
is defined as a call to join and is fixed with it.

The packed lane is untouched: a slot the snapshot holds is still one bounds
compare, one array load and one null test, with no extra field read and no
call. Only a hole - or an index past the snapshot - reaches the new branch, and
only once the realm's array prototype chain has actually gained an index
property does that branch walk anything.

Three sibling divergences share the shape and are deliberately left alone here:
toLocaleString reads through TryGetValue rather than Get; forEach and the other
callback generics skip a hole the prototype gained mid-iteration because
HasProperty answers from the same snapshot; and a side effect that reallocates
the backing store leaves the snapshot itself stale, which cannot be fixed
without a live _dense load on the per-element path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@lahma
lahma force-pushed the fix/array-join-hole-reread branch from 399e5cd to 49bd6be Compare August 12, 2026 17:06
@lahma

lahma commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Benchmark gate — measured, reworked once, now PASS

The first measurement caught a real cost: ArrayHoleTraversalBenchmark.JoinHoley at +54.9% — every hole paid a non-inlined JsArray.Get call even when CanUseFastAccess proves nothing can shadow it. The fix now answers a hole inline while that flag holds (re-read per hole, because a side effect during the very join is exactly what can clear it) and only falls back to the spec's chain-walking read once it is gone.

Re-gated after the rework:

Row base cand Δ
JoinHoley 536.2 µs 554.7 µs +3.4%
JoinDense 945.2 µs 949.3 µs +0.4%
SumHoley / SumDense / InOperatorHoley / IndexOfMissHoley −0.7% to −0.2%
IndexOfMissDense (untouched) −6.9% (scatter)

The residual +3.4% on the hole-heavy row is the honest cost of the correctness fix — per-hole flag read and branch where there used to be nothing — disclosed rather than hidden. Allocation byte-identical everywhere. ArrayTests 167/167 on both TFMs after the rework; the amended commit is force-pushed.

@lahma
lahma merged commit 2d4584d into sebastienros:main Aug 13, 2026
5 checks passed
@lahma
lahma deleted the fix/array-join-hole-reread branch August 13, 2026 16:14
lahma added a commit that referenced this pull request Aug 13, 2026
Full re-measure of both suites (script + interop, one session, default job, idle
machine: 96 script rows and 20 interop rows) on the 4.16.0 release candidate
7b56c83. Every figure in the narrative sections is recomputed from the new
reports; no number is carried forward from the 4.15.0 tables.

Where the table stands now:

- Jint is fastest outright on 5 of 12 scripts (minimal ~345x V8's compiled lane,
  evaluation-modern ~80x, linq-js ~6.7x, dromaeo-core-eval-modern ~5%,
  dromaeo-object-regexp-modern 1.25x ahead of V8's fresh-context lane and 1.46x
  ahead of its compiled lane), fastest managed engine on 10 of 12, and fastest
  interpreter on all 12
- V8 keeps the tight-loop rows: base64 9.8x, object-string 6.6x, stopwatch 6.0x,
  3d-cube 3.4x, json-parse 2.2x, plus narrow leads on object-array (1.08x) and
  array-stress (1.09x) - array-stress being the one script row that changes
  hands, out of the rank-1 tie it held at 4.15.0
- Allocation: Jint is lowest of the managed engines on 10 of 12 scripts (Okojo
  on object-array, NiL.JS on minimal) and on all four interop rows, 3.9x-12.4x
  under the nearest managed competitor there
- Interop: rank 1 on string-passing, and back into a rank-1 tie with NiL.JS on
  collection-traversal (1,251.0 vs 1,242.7 us, 0.7% apart); rank 2 on
  method-calls (NiL.JS ahead) and property-access (YantraJS by 2.6%). Plain
  ClearScript costs 8.6x-11.2x against Jint, FastProxy 3.4x-7.0x

Adds a "What changed for 4.16.0" section: proper tail calls (#2975, which
measured -15.6% time and -40.4% allocation on the Jint-only controlflow-recursive
row against 4.15.3), the fast-call lane's growth (#2968, #2980, #2984), the
wrapped-dictionary probe lane (#2969), and the disclosed cost of the join-hole
re-read (#3003, +3.4% on hole-heavy joins). The comparison against the 4.15.0
tables is stated as directional only - the two sessions ran on .NET 10.0.10 ->
10.0.11 with YantraJS 1.2.419 -> 1.2.422 in between, so no row-for-row delta is
claimed - and the two stale prose claims naming 4.15.0 outside the history
sections are refreshed from this session's data.

Environment: AMD Ryzen 9 5950X, .NET 10.0.11 (SDK 10.0.400), BenchmarkDotNet
0.15.8, default job, otherwise idle machine. ClearScript's V8 lanes land within
~3% of the 2026-07-28 session on the identical package.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant