perf(hashset): store entries as struct-of-arrays

[bobzhang]

Second of two PRs splitting #3712. Stacked on #4125 — review that one first; this diff is hongbo/hashset-unchecked..hongbo/hashset-soa.

Original layout work by @mizchi, carried over with authorship.

The change

FixedArray[Entry[K]?] with a heap-allocated Entry { psl, hash, key } becomes three parallel arrays:

psls   : FixedArray[Int]        // -1 marks an empty slot
hashes : FixedArray[Int]
keys   : UninitializedArray[K]

psls[i] == -1 is the empty-slot sentinel — a real probe sequence length is always >= 0 — so no separate occupancy array is needed. Inserting writes three slots and allocates nothing. Vacated slots get set_null so the key stays collectable. Same Robin Hood algorithm, same observable behaviour, pkg.generated.mbti unchanged.

The unchecked accessors are declared package-locally as methods on UninitializedArray in hashset/types.mbt rather than exposed from builtin. That is deliberate: set_null and unsafe_set can corrupt memory or resurrect a freed slot, so the fewer packages able to name them, the better.

Measurements

n=50000, against its parent #4125, interleaved on one machine in a single session:

backend	op	parent (boxed + unchecked)	this (SoA)	change
native	add	2.29 ms	1.49 ms	35% faster
native	contains	871 µs	792 µs	9% faster
wasm-gc	add	2.38 ms	1.98 ms	17% faster
wasm-gc	contains	1.15 ms	1.06 ms	8% faster
js	add	2.55 ms	3.02 ms	18% slower
js	contains	1.05 ms	1.05 ms	unchanged

Against origin/main, the two PRs together give native add 2.35 → 1.49 ms, wasm-gc 2.30 → 1.98 ms, js 3.08 → 3.02 ms.

That js number is the whole reason for the split. Measured against main, the combined #3712 looked like "big native win, js unchanged" — but the js parity was the probing win cancelling out the layout's cost, not the layout being free. Only separating them shows the layout costs js 18% on its own.

Where the js cost actually comes from

I attributed it to the three-array probe. The review disproved that with generated-code measurements and localised it to grow: allocating and initialising three backing stores costs ~0.68 ms against ~0.25 ms for one, while the rehash loops themselves came out about equal.

That materially narrows the affected workload:

• unknown-size incremental construction regresses ~18–19% for integers, ~14% for short strings — which is exactly what this benchmark measures, i.e. the worst case;
• pre-sized construction is near parity (+2.8%, or roughly equal with construction excluded);
• lookup is neutral for integers and modestly faster for strings.

So the cost is a construction-and-growth cost on js, not a steady-state one. A caller who sizes the set up front barely pays it.

Recommendation

The review recommends landing as is — option (a) — rather than a #cfg(target="js") fork keeping the boxed layout on js:

Duplicating the storage engine — and potentially doing the same for Map/Set — adds more maintenance and semantic-drift risk than the growth-only JS cost justifies.

I agree, with the caveat that this is a judgement call and the numbers above are what it rests on. The final stack is near parity with main on js while gaining 35% on native and 17% on wasm-gc.

Review

Reviewed by Codex CLI at ultra reasoning effort — approved, verdict below. Two non-blocking test gaps it names and I have not closed: reclamation is argued from emitted code rather than a weak-reference test (see the discussion of the liveness gap on #3712), and remove-then-grow plus wraparound shift-back deserve focused regression tests.

[bobzhang]

Codex CLI review (ultra reasoning effort)

Approved, with a correction to my attribution of the js cost that materially narrows which workloads it affects.

I approve commit 45ae2236 as-is over parent fdd064cd. No correctness, UB, or API blockers found.

The JavaScript attribution needs one refinement: the high-level explanation is right — JS gains nothing from removing RC and V8 allocates small entry objects cheaply — but the residual cost is not primarily three-array probing. Generated-code measurements reproduced the regression (2.477 ms boxed versus 2.953 ms SoA, +19.2%) and isolated most of it to grow: allocating/initializing three backing stores costs roughly 0.68 ms versus 0.25 ms for one; the actual rehash loops were approximately equal. Packing keys did not help.

That distinction narrows the affected workload:

• Unknown-size incremental construction regresses: integers around 18–19%, short strings around 14%.
• Pre-sized construction was near parity (+2.8%, or approximately equal with construction excluded).
• Lookup was neutral for integers and modestly faster for strings.

I recommend option (a), not a target-specific fork. The final stack remains near parity with main on JS while gaining 35% native and 17% wasm-gc insertion performance. Duplicating the storage engine — and potentially doing the same for Map/Set — adds more maintenance and semantic-drift risk than the growth-only JS cost justifies. Map and Set should still be benchmarked independently, including incremental growth, pre-sized construction, short strings, lookup, and mutation-heavy workloads.

The layout audit was clean:

• -1 cannot collide with a real PSL, and every key read is dominated by an occupied-slot check.
• push_away preserves Robin Hood displacement.
• shift_back nulls the final duplicate slot; clear releases every occupied key.
• Grow and copy read only initialized key slots; retain and wraparound traversal preserve the invariant.
• The private foreign-type methods exactly match the builtin intrinsic signatures and remain absent from pkg.generated.mbti, which is byte-identical to the parent.

Non-blocking test gaps: reclamation is verified through emitted-code inspection rather than weak-reference/RC tests; remove-then-grow and wraparound shift-back deserve focused regression tests.

Signed-off-by: Codex CLI codex@openai.com

Worth drawing out, because it changes how this PR should be read: the benchmark in this repo measures the worst case for the change. HashSet([]) followed by 50000 adds is unknown-size incremental construction, which is precisely the workload that pays the growth cost. A caller who pre-sizes the set is at parity on js, and lookup is neutral-to-better everywhere.

It also flags the obvious next question, which I will answer with data rather than by analogy: Map and Set are the linked containers, and their SoA conversion is a bigger change than this one because the intrusive list currently relies on entry identity. They get benchmarked on their own — incremental growth, pre-sized construction, short strings, lookup and mutation-heavy — before anyone decides.

[coveralls]

Coverage Report for CI Build 6299

Coverage increased (+0.004%) to 90.881%

Details

• Coverage increased (+0.004%) from the base build.
• Patch coverage: 6 uncovered changes across 1 file (70 of 76 lines covered, 92.11%).
• No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
hashset/hashset.mbt	76	70	92.11%

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	18466
Covered Lines:	16782
Line Coverage:	90.88%
Coverage Strength:	305658.84 hits per line

---

💛 - Coveralls

[Copilot]

Pull request overview

Refactors @hashset.HashSet’s internal table storage from an array of boxed Entry objects to a struct-of-arrays layout (psls/hashes/keys) to avoid per-insert allocations while keeping the Robin Hood hashing algorithm and public API unchanged.

Changes:

• Replaces entries: FixedArray[Entry[K]?] with parallel arrays psls, hashes, and keys using empty_psl = -1 as the empty-slot sentinel.
• Adds package-local unchecked accessors (unsafe_get, unsafe_set, set_null) for UninitializedArray key storage and updates probe/rehash/shift-back logic accordingly.
• Extends test coverage for wraparound collision handling, clear buffer reuse, and sparse-copy correctness.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
hashset/types.mbt	Introduces empty_psl, package-local UninitializedArray unchecked accessors, and updates HashSet storage fields to SoA.
hashset/hashset.mbt	Rewrites core HashSet operations to use SoA storage, ensures deletions null out keys, and adds/updates regression tests.

Suppressed comments (2)

hashset/hashset.mbt:236

• shift_back also uses mask-derived indices (next = (cur + 1) & capacity_mask), but it reads/writes psls with checked indexing. Using unsafe_get/unsafe_set here avoids redundant bounds checks and matches the safety invariant documented elsewhere in the file.

    let next = (cur + 1) & self.capacity_mask
    let next_psl = self.psls[next]
    if next_psl == empty_psl || next_psl == 0 {
      self.psls[cur] = empty_psl

hashset/hashset.mbt:243

• In shift_back, next is already proven in-bounds by masking, and next_psl != empty_psl guarantees keys[next] is initialized. Using unsafe_get for the hashes/keys reads avoids reintroducing bounds checks in this relocation loop.

      self.set_slot(cur, next_psl - 1, self.hashes[next], self.keys[next])

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bobzhang]

Rebased onto current main and re-measured. The conclusion changed: this is now faster on every backend except js add.

Three things moved since the last revision.

1. The baseline was measuring a bug

The previous numbers compared against HashSet::copy as it stood before #4130 — which blitted entry references rather than copying them, so it was doing far less work and was incorrect. My earlier report of "copy 3.4× slower on both collected backends" was comparing real copying against a reference blit. That comparison is void.

#4130 fixed the underlying defect on main (emptying a copy of a 32-element set left all but two of the original's keys unreachable). The baseline below is main with a correct copy, so both sides now do the same work.

2. Two more silently-reverted paths

The layout work predates the unchecked-probing commit, so wherever it rewrote a function it wrote the pre-unchecked form back. remove and shift_back were caught last round; iter had gone the same way and is now restored.

Rather than spot-check again, I enumerated every function that probes without bounds checks on main — add_with_hash, push_away, set_entry, contains, remove, shift_back, rehash_place_entry, iter — and confirmed this branch matches all eight, plus grow and copy. each, eachi, clear, to_array, retain and _debug_entries are checked on main and stay checked here.

Twice now I have found only the reverted functions a benchmark happened to cover, so the review has been asked to redo that comparison independently.

3. copy no longer does redundant work

The metadata arrays are built with copy() instead of make-then-blit — one allocate-and-copy each rather than allocate, fill, then overwrite — which on js lowers to Array.slice.

Measurements

n=50000, against corrected main, interleaved on one machine in a single session:

backend	op	main	this branch	change
native	add	2.28 ms	1.41 ms	38% faster
native	contains	829 µs	745 µs	10% faster
native	copy	472 µs	71 µs	85% faster
native	removal	1.07 ms	684 µs	36% faster
wasm-gc	add	2.19 ms	1.89 ms	14% faster
wasm-gc	contains	1.12 ms	989 µs	12% faster
wasm-gc	copy	368 µs	238 µs	35% faster
wasm-gc	removal	1.12 ms	1.06 ms	5% faster
js	add	2.46 ms	2.84 ms	15% slower
js	contains	1.00 ms	987 µs	unchanged
js	copy	434 µs	352 µs	19% faster
js	removal	1.08 ms	998 µs	7% faster

Removal is isolated by difference, from a benchmark that copies a prebuilt set inside the timed closure plus a companion that times the copy alone — @bench.T has no per-iteration setup hook, so it cannot be measured directly.

Where that leaves the decision

The only regression left is js add, at 15%, and it is growth-bound: the layout allocates three backing stores where the boxed one allocates a single array, and js has no per-entry malloc or reference-counting churn for the layout to remove in exchange. Pre-sized construction was previously measured near parity.

Against that: native gains 38% on insertion and 85% on copy, wasm-gc gains across the board, and removal improves everywhere. I read that as landing without a target-specific fork, but the decision is the maintainer's and these are the numbers it should rest on.

Codex CLI verification at xhigh is running; its verdict will follow. I have specifically asked it to challenge the native copy figure — a 6.6× improvement deserves scepticism before it stands in a commit message — and to redo the unchecked-parity comparison independently.

[bobzhang]

Codex CLI verification (xhigh) — approved

No blocking findings. I would land 6ec6a102 as is.

• Unchecked-access parity is confirmed: main's eight paths all remain unchecked — add_with_hash, push_away, set_slot/set_entry, contains, remove, shift_back, rehash_place_entry, and iter. grow and copy are newly unchecked with valid bounds and occupancy proofs. clear uses raw set_null, but its scan remains bounds-checked and sound.
• copy creates three independent backing arrays. It never reads an unoccupied source key; it initializes only destination slots corresponding to occupied source slots. Shallow sharing of key values is normal container-copy semantics.
• The native 472→71 µs result is plausible. At 50,000 elements the table capacity is 65,536: the new implementation performs roughly memory-bandwidth-scale array copying and a key loop, while corrected main allocates and later reclaims 50,000 Entry objects. The benchmark measures copy plus temporary-copy reclamation on native, but does so symmetrically, so the comparison remains fair.
• The removal benchmark now copies an immutable template correctly. Subtracting the separate copy benchmark introduces some measurement error, but not enough to challenge the reported deltas.
• A JS-specific representation fork is not warranted for a 15% growth-heavy add regression when lookup is unchanged and copy/removal improve.
• Non-blocking residual: to_json now materializes an intermediate Array[K], potentially adding allocation during serialization.

Signed-off-by: Codex CLI codex@openai.com

The independent parity check is the one I most wanted, since that defect class had already slipped twice. It agrees with mine: all eight of main's unchecked paths are intact, and the two additions (grow, copy) carry their own proofs.

The to_json residual was real and is fixed in 82a5cf29. Main builds a single Json array via comprehension; my version went through to_array(), materializing an Array[X] before the Array[Json]. It now builds directly from the backing arrays:

[
  for i in 0..<self.capacity
  if self.psls[i] != empty_psl =>
    self.keys[i]
]

Revalidated after that change: moon check --deny-warn clean on native, js and wasm-gc; moon test 7546/7546 and 7487/7487 on js; pkg.generated.mbti unchanged.

That leaves this ready as far as I can take it. The remaining call is yours: land as is with js add 15% slower on growth-heavy insertion, against 38% faster insertion and 85% faster copy on native, gains across the board on wasm-gc, and lookup unchanged everywhere.

[bobzhang]

Why js add is slower — and a retraction

I asked Codex CLI (xhigh) to explain the js regression rather than accept my own account of it, because I had been wrong about it three times. The answer changes the landing recommendation, and invalidates a claim in my previous commit message.

It is not growth-bound. That was my explanation and the basis for "callers who pre-size avoid it". A pre-sized benchmark refutes it: with the table sized up front so no growth occurs at all, the gap persists and widens in relative terms.

js add, n=50000	main (boxed)	this branch	delta
growing from 8	2.42 ms	2.92 ms	+21%
pre-sized, no growth	862 µs	1.24 ms	+44%

Also ruled out, each by measurement rather than argument: holey keys (patching the generated JS to pack it recovers nothing); redundant array-reference loads (hoisting all three into locals moved nothing — V8 already hoists them); inlining (V8's trace shows add, add_with_hash, set_slot and the displacement path all inlined); element-kind churn (psls/hashes settle as packed SMI, keys as holey SMI, no repeated transitions); write barriers (everything is SMI here, and the boxed layout is the one doing a pointer store).

The actual mechanism

It is the count of random keyed stores into separate backing arrays. Instrumented over the benchmark:

• boxed — 53,586 random entries[idx] stores, plus 50,000 sequential Entry allocations
• SoA — 160,758 random keyed stores spread across psls, hashes and keys

Causal patches to the generated program, pre-sized medians:

generated-JS variant	time
boxed reproduction	0.90 ms
boxed + one random shadow-array store	1.20 ms
boxed + two random shadow-array stores	1.40 ms
boxed + two extra in-object fields	0.97 ms
current SoA	1.26 ms
SoA with psls/hashes as Int32Array	0.60–0.64 ms

Adding the same extra random stores to the boxed version reproduces and overshoots the regression, while adding the same number of sequential writes to a freshly allocated object costs almost nothing. V8's bump-pointer allocation plus constructor-field initialisation is simply cheaper than two extra random generic-array stores.

The last row is the important one: this is not inherent to struct-of-arrays or to garbage collection. It follows from FixedArray[Int] lowering to an ordinary JS array of tagged machine words. (This Node build is arm64 with pointer compression off, so a tagged SMI slot is 8 bytes where Int32Array is 4.) With typed arrays for the metadata, SoA becomes faster than boxed on js — 0.60 ms against 0.90 ms pre-sized, and 2.24 ms against 2.42 ms growing.

Recommendation, revised

Codex's verdict, and I agree with it:

Yes, my recommendation changes: I would not land 82a5cf29 as-is. The JS regression is on the steady-state primary operation and cannot be avoided by pre-sizing; pre-sizing exposes it more sharply. I would not maintain an entirely separate boxed JS HashSet, but I would hold this commit for either the narrow typed-metadata specialization or a fully validated two-array representation.

Two routes it identified, both better than a representation fork:

1. Int32Array for the metadata on js. Narrow — it touches the private metadata allocator and accessors, not the HashSet algorithm. Fully recovered the regression in the generated-JS experiment.
2. A portable two-array design. Reserve an empty sentinel in hashes, remap that one hash value, and derive PSL arithmetically as (idx - (hash & mask)) & mask, eliminating the psls array entirely. A generated-JS proof of concept took pre-sized add from 1.26 ms to ~0.95 ms. This is portable, so it should help wasm-gc and native too — but it needs the full grow/remove/contains/copy implementation and cross-backend benchmarking before it counts as a solution.

The commit message has been corrected: it previously asserted the regression was growth-bound, which is false. This PR should not be merged in its current form. I would rather hold it than land a known regression on the primary operation of a core container on the basis of an explanation that turned out to be wrong.