perf(hashset): store entries as struct-of-arrays to avoid per-insert alloc

[mizchi]

Motivation

HashSet stored its table as FixedArray[Entry[K]?], where
Entry { psl, hash, key } is a heap-allocated struct. Every newly inserted key
therefore allocates one Entry object. Profiling HashSet::add (native, Time
Profiler) attributed ~11% of the time to the per-entry malloc, on top of the
reference-counting churn (incref/decref/drop) of the boxed entries.

Change

Replace the array of boxed entries with a struct-of-arrays layout:

psls   : FixedArray[Int]        // probe sequence length; -1 marks an empty slot
hashes : FixedArray[Int]        // cached key hash for occupied slots
keys   : UninitializedArray[K]  // key storage for occupied slots

psls[i] == -1 is the empty-slot sentinel — a real probe-sequence length is
always >= 0, so no extra occupancy array is needed. Inserting a key now writes
three array slots with no allocation. Vacated slots (remove / shift-back / clear)
have their key set_null-ed so it stays collectable. UninitializedArray[K] is
the same primitive already used by @deque.

Same Robin Hood algorithm, same observable behavior — only the storage layout
changes.

Benchmark

hashset/hashset_bench_test.mbt (native, n=50000):

op	before	after
add	1.95 ms	1.38 ms	~29% faster
contains	657 µs	675 µs	unchanged (read path does not allocate)

The add win exceeds the 11% the profiler attributed to malloc alone, because
the boxed-entry reference-counting churn disappears too. contains is on the
read path and neither layout allocates, so it is unchanged.

Validation

moon test -p hashset passes on native, wasm-gc, wasm and js (132 each).
moon check is clean and the package .mbti is unchanged (the struct fields
are private, so this is a pure internal change).

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR refactors @hashset.HashSet’s internal table storage from an array of boxed Entry objects to a struct-of-arrays layout (psls / hashes / keys) to eliminate per-insert heap allocations while preserving the existing Robin Hood hashing behavior.

Changes:

• Replaces entries: FixedArray[Entry[K]?] with psls: FixedArray[Int], hashes: FixedArray[Int], and keys: UninitializedArray[K] (using empty_psl = -1 as the empty-slot sentinel).
• Updates all core operations (add, contains, remove, rehash/grow, iteration, copy, debug helpers) to operate on the new storage layout.
• Adds benchmarks (hashset_bench_test.mbt) and includes the bench dependency for tests in hashset/moon.pkg.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
hashset/types.mbt	Introduces the new table layout primitives (empty_psl, set_null) and updates HashSet’s internal fields to the struct-of-arrays design.
hashset/hashset.mbt	Rewrites all HashSet operations to use psls/hashes/keys storage, removing per-entry boxing/allocation.
hashset/moon.pkg	Adds the bench package dependency for test/benchmark support.
hashset/hashset_bench_test.mbt	Adds microbenchmarks for HashSet::add and HashSet::contains.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mizchi]

Addressed the clear feedback: it now reuses the existing buffers and only nulls the occupied key slots (then resets their psls) instead of reallocating keys, matching the "keeps the allocated space" docstring. Tests pass on native/wasm-gc/wasm/js.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f6e67c63a1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[bobzhang]

Rebased onto main and benchmarked on all three backends. The layout change is a real win on native and wasm-gc, but it regressed js add by ~28%, which the original benchmark did not catch because only native was measured. That is now fixed; details below.

Rebase

Four conflicts, all from main moving under the PR:

• hashset/moon.pkg — kept both quickcheck (main) and bench (this PR) in the test imports.
• hashset/hashset.mbt — main has since added #owned(entry) to push_away, set_entry and rehash_place_entry. All three name an entry parameter that no longer exists under struct-of-arrays, so the annotations are dropped; set_entry is the PR's set_slot.
• hashset/hashset_bench_test.mbt — unqualified HashSet now trips warning 0025 (test_unqualified_package), a lint that did not exist in June. Qualified, with the then-redundant annotation removed to satisfy unnecessary_annotation.
• One stray marker in retain, whose merged body already matched this PR.

hashset/pkg.generated.mbti is unchanged — this stays a pure internal change. moon test hashset passes 76/76 on native, wasm-gc, wasm and js; full moon test is 7526/7526.

The js regression

hashset_bench_test.mbt, n=50000, this branch vs origin/main running the same bench file, interleaved on one machine:

backend	op	main (boxed)	this PR, as opened	change
native	add	2.28 ms	1.66 ms	27% faster
wasm-gc	add	2.19 ms	1.93 ms	12% faster
js	add	2.78 / 2.93 ms	3.68 / 3.72 ms	~28% slower
native	contains	871 µs	759 µs	13% faster
wasm-gc	contains	1.07 ms	990 µs	7% faster
js	contains	1.19 ms	976 / 982 µs	18% faster

Two independent runs per side on js; the ranges do not overlap (main 2.57–3.17 ms, PR 3.55–3.84 ms), so the regression is real.

The reason is structural rather than incidental. Struct-of-arrays replaces one checked array access per probe step with three — and on js the costs the layout exists to remove, per-entry malloc and reference-counting churn, do not exist in the first place. V8 allocates the small Entry objects cheaply, so the trade is all cost and no benefit there.

I first guessed the cause was UninitializedArray::make lowering to a bare new Array(n) (holey in V8) while psls and hashes get .fill()ed and stay packed. That guess was wrong: patching the generated JS to fill the keys array, and confirming V8 then reports it packed, recovers nothing. Removing the generated bounds checks is what recovers the time.

Fix: unsafe_get / unsafe_set on the insertion path

The last commit here applies the treatment contains already had. Every probe index starts as hash & capacity_mask and each step re-masks it, so it is in bounds for all three arrays; a non-empty PSL guarantees set_slot already initialized that key slot. That invariant is written out as a SAFETY comment at each site — add_with_hash, push_away, rehash_place_entry, grow's rehash loop, and set_slot.

backend	main	SoA as opened	SoA + unchecked
native	2.28 ms	1.66 ms	1.41 ms
wasm-gc	2.19 ms	1.93 ms	1.89 ms
js	2.78 / 2.93 ms	3.68 / 3.72 ms	2.88 ms

js is back inside its own baseline range, and native picks up a further 15% — 38% against main overall. contains is untouched by that commit (native 760 µs, wasm-gc 1.01 ms, js 1.01 ms).

@mizchi — that last commit is mine, pushed here since the PR allows maintainer edits. Drop it if you would rather take a different route; the measurements above are the argument for it, not the code itself.

Codex CLI review (xhigh reasoning effort)

Two rounds. Round 1 raised the js regression as a P1; round 2, after the fix, approved.

Round 1 disproved my first explanation experimentally, which is why the fix is what it is:

The holey-array fact is right, but it is not the main explanation. On Node 25.8.2 / V8 14.1, the generated bare new Array(n) is holey. However, I changed the generated benchmark in-memory to use .fill(null), verified that V8 then reported it packed, and ran it interleaved: original medians 3.60 ms, 3.62 ms; packed-key medians 3.66 ms, 3.68 ms. Packing keys did not recover performance. […] Conversely, removing generated bounds checks gave about 2.79 ms, close to your 2.78–2.93 ms main baseline. Removing only the three checks in generated set_slot recovered roughly 10%. That points to the multi-array checked access pattern, plus more expensive growth/rehashing, as the dominant cause.

It also noted the js contains win comes from the existing unsafe_get commit rather than from the layout, and corrected me on set_null: it does not compile away on js — it emits keys[i] = null and is required to release the reference. It was simply dead-code-eliminated out of the release bench, where remove/clear are never called.

Round 2, on the unchecked-access commit:

No blocking findings. I approve the PR.

• Every set_slot index is masked: directly from an insertion/rehash probe, from push_away's re-masked loop, or from shift_back, whose initial and subsequent cur values are masked.
• During grow, the new arrays, capacity, and mask are installed before rehashing begins. The old-array loop uses only i < old_capacity; rehash operations use only the new arrays and mask. There is no old/new index mismatch.
• add_with_hash reads self.capacity_mask only after grow() returns, so it cannot retain the old mask.
• The initialized-key invariant survives shift_back, clear, retain, and copy. Slots are nullified only after their PSL becomes empty; copied occupied slots receive their key before the new set is returned.
• Stale hashes in empty slots are harmless because every relevant hash read is dominated by a non-empty PSL check.
• unsafe_set_key's signature exactly matches the builtin UninitializedArray::unsafe_set binding to %fixedarray.unsafe_set.
• set_slot writes PSL before key, but there is no callback or suspension between those stores, and HashSet explicitly does not support concurrent access.

Tests alone cannot establish unchecked safety. The meaningful evidence is the exhaustive caller/state-transition proof above. […]

The round-one P1 no longer stands: JS performance is back within baseline variance, while native improved further. The whole PR is landable.

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

Worth repeating its caveat: for unchecked indexing, a green test suite is weak evidence — an out-of-bounds access is undefined behaviour, not a failure. What justifies this commit is the caller/state-transition argument, not the 76/76.

One open P3 it raised against the PR as a whole, which I have not addressed: deleting both set_null calls would leave every test passing while removed keys stay reachable until the set itself dies. There is no liveness test for that.

[bobzhang]

Two follow-ups.

1. The unsafe primitives are methods now (b3668635). unsafe_get, unsafe_set and set_null already existed on UninitializedArray in builtin, just package-private, which is why this PR re-declared each as a local free-function extern. They are now pub behind #internal(unsafe, ...) + #doc(hidden) — the same treatment FixedArray::unsafe_get/unsafe_set and UninitializedArray::unsafe_blit already carry — and the three shims in hashset/types.mbt are gone. Call sites now read in the same style as the FixedArray ones beside them:

self.keys.unsafe_get(idx)        // was unsafe_get_initialized_key(self.keys, idx)
self.keys.unsafe_set(idx, key)   // was unsafe_set_key(self.keys, idx, key)
self.keys.set_null(cur)          // was set_null(self.keys, cur)

set_null also moves from arraycore_nonjs.mbt to uninitialized_array.mbt: the primitive works on every backend — this PR already relied on it on js — and only Array's own use of it was non-js specific. Both generated interfaces are unchanged, since #doc(hidden) keeps these off the public surface.

2. Corrected benchmark table. The numbers I posted above were collected across several sessions and the machine drifted about 5% between them. Re-measured back-to-back in one session, main versus this branch's head:

backend	op	main (boxed)	this branch	change
native	add	2.40 ms	1.49 ms	38% faster
native	contains	910 µs	794 µs	13% faster
js	add	3.14 ms	3.01 ms	parity
js	contains	1.27 ms	1.04 ms	18% faster

The ratios are the same as before and every conclusion stands — the js regression is gone and native is 38% ahead — but these absolutes are the reproducible ones. I also A/B'd the refactor in commit 2 against commit 1 directly: 1.48 ms vs 1.49 ms, so moving to methods costs nothing.

[bobzhang]

Correction to my previous comment: I have reverted the builtin change. The PR touches hashset only again.

Exposing UninitializedArray::unsafe_set and set_null as pub was the wrong call even behind #doc(hidden) — hidden is not inaccessible, and those two can corrupt memory or resurrect a freed slot, so publishing them to every downstream package to save three declarations in one package is a bad trade.

The OO style is kept without that cost: the accessors are declared as methods on UninitializedArray package-locally inside hashset, which the compiler accepts and which goes no further than this package.

fn[K] UninitializedArray::unsafe_get(self : UninitializedArray[K], idx : Int) -> K = "%fixedarray.unsafe_get"
fn[K] UninitializedArray::unsafe_set(self : UninitializedArray[K], idx : Int, key : K) -> Unit = "%fixedarray.unsafe_set"
fn[K] UninitializedArray::set_null(self : UninitializedArray[K], idx : Int) -> Unit = "%fixedarray.set_null"

Call sites read the same as before and the same as the FixedArray ones beside them — self.keys.unsafe_get(idx), self.keys.unsafe_set(idx, key), self.keys.set_null(cur) — while builtin is byte-identical to main. Both generated interfaces unchanged; moon test 7526/7526, hashset 76/76 on native, wasm-gc, wasm and js.

On the liveness P3: I do not think a test is the right answer there. A real liveness assertion needs weak references or finalization, which is not portable across the four backends, and the leak it guards against is bounded — at most one dead key per vacated slot, released at the next grow. What actually threatens the invariant is a person reading shift_back and deleting a line that looks redundant next to the psls[cur] = empty_psl above it. So the guard is at that line, in the source:

self.psls[cur] = empty_psl
// Not redundant with the line above: the PSL marks the slot free, but
// the key slot would still hold the removed key alive until something
// overwrites it, retaining up to one dead key per vacated slot.
self.keys.set_null(cur)

clear already carried an equivalent note. Happy to add a real liveness test if the runtime grows the primitives for one.

[bobzhang]

Superseded by a split, per review: this PR bundled two independent changes and each was masking the other's cost.

• perf(hashset): probe with unchecked array access #4125 — unchecked probing (no layout change). js add 18% faster, contains 17% faster; native and wasm-gc neutral. Approved by Codex CLI at ultra effort.
• struct-of-arrays layout, stacked on perf(hashset): probe with unchecked array access #4125, opening shortly.

Splitting them changed the conclusion. Measured against main, the combined PR looked like "native much faster, js unchanged". Measured against the probing half alone, the layout is 35% faster on native, 17% faster on wasm-gc, and 18% slower on js — the probing win had been hiding the layout's js cost. That trade is a real decision, and it deserves its own PR and its own numbers rather than riding along.

@mizchi — thank you, the layout work and the benchmark file are both carried over with authorship credit, and the split is about making each half defensible on its own evidence rather than about the work itself. The remaining question on the layout half is whether native +35% is worth js -18%, or whether it wants a #cfg(target="js") split; that will be decided on the new PR.