From a55bdb473772c24a01a6cc18e5c304b809f5f870 Mon Sep 17 00:00:00 2001 From: mizchi Date: Fri, 26 Jun 2026 19:22:37 +0900 Subject: [PATCH 1/6] perf(hashset): store entries as struct-of-arrays to avoid per-insert alloc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HashSet stored its table as `FixedArray[Entry[K]?]`, so every newly inserted key allocated a heap `Entry { psl, hash, key }` object. Profiling `HashSet::add` showed ~11% of time in that per-entry malloc, on top of the reference-counting churn of boxed entries. Replace the array of boxed entries with a struct-of-arrays layout: `psls` / `hashes : FixedArray[Int]` and `keys : UninitializedArray[K]`, using `psls[i] == -1` as the empty-slot sentinel (a real probe-sequence length is always >= 0). Inserting a key now writes three array slots with no allocation; vacated slots have their key nulled so it stays collectable. Bench (native, n=50000): add: 1.95 ms -> 1.38 ms (~29% faster) contains: 657 µs -> 675 µs (unchanged; the read path does not allocate) Adds hashset/hashset_bench_test.mbt. Co-Authored-By: Claude Opus 4.8 (1M context) --- hashset/hashset.mbt | 233 ++++++++++++++++++--------------- hashset/hashset_bench_test.mbt | 44 +++++++ hashset/moon.pkg | 1 + hashset/types.mbt | 25 ++-- 4 files changed, 189 insertions(+), 114 deletions(-) create mode 100644 hashset/hashset_bench_test.mbt diff --git a/hashset/hashset.mbt b/hashset/hashset.mbt index e9d9f6340e..070a25636d 100644 --- a/hashset/hashset.mbt +++ b/hashset/hashset.mbt @@ -25,7 +25,9 @@ fn[K] new_hashset(capacity : Int) -> HashSet[K] { capacity, capacity_mask: capacity - 1, grow_at: calc_grow_threshold(capacity), - entries: FixedArray::make(capacity, None), + psls: FixedArray::make(capacity, empty_psl), + hashes: FixedArray::make(capacity, 0), + keys: UninitializedArray::make(capacity), } } @@ -94,62 +96,63 @@ fn[K : Eq] HashSet::add_with_hash( self.grow() } let (idx, psl) = for psl = 0, idx = hash & self.capacity_mask { - match self.entries[idx] { - None => break (idx, psl) - Some(curr_entry) => { - if curr_entry.hash == hash && curr_entry.key == key { - return - } - if psl > curr_entry.psl { - self.push_away(idx, curr_entry) - break (idx, psl) - } - continue psl + 1, (idx + 1) & self.capacity_mask - } + let curr_psl = self.psls[idx] + if curr_psl == empty_psl { + break (idx, psl) + } + if self.hashes[idx] == hash && self.keys[idx] == key { + return } + if psl > curr_psl { + // Displace the richer occupant of `idx`, then write the new key here. + self.push_away(idx, curr_psl, self.hashes[idx], self.keys[idx]) + break (idx, psl) + } + continue psl + 1, (idx + 1) & self.capacity_mask } - let entry = { psl, key, hash } - self.set_entry(entry, idx) + self.set_slot(idx, psl, hash, key) self.size += 1 } ///| -#owned(entry) +// Relocates the key triple `(psl, hash, key)` -- which has just been bumped out +// of an earlier slot -- to a later slot, Robin-Hood style, starting from the +// slot after `idx`. fn[K] HashSet::push_away( self : HashSet[K], idx : Int, - entry : Entry[K], + psl : Int, + hash : Int, + key : K, ) -> Unit { - for psl = entry.psl + 1, idx = (idx + 1) & self.capacity_mask, entry = entry { - match self.entries[idx] { - None => { - entry.psl = psl - self.set_entry(entry, idx) - break - } - Some(curr_entry) => - if psl > curr_entry.psl { - entry.psl = psl - self.set_entry(entry, idx) - continue curr_entry.psl + 1, - (idx + 1) & self.capacity_mask, - curr_entry - } else { - continue psl + 1, (idx + 1) & self.capacity_mask, entry - } + for psl = psl + 1, idx = (idx + 1) & self.capacity_mask, hash = hash, key = key { + let curr_psl = self.psls[idx] + if curr_psl == empty_psl { + self.set_slot(idx, psl, hash, key) + break + } else if psl > curr_psl { + let curr_hash = self.hashes[idx] + let curr_key = self.keys[idx] + self.set_slot(idx, psl, hash, key) + continue curr_psl + 1, (idx + 1) & self.capacity_mask, curr_hash, curr_key + } else { + continue psl + 1, (idx + 1) & self.capacity_mask, hash, key } } } ///| #inline -#owned(entry) -fn[K] HashSet::set_entry( +fn[K] HashSet::set_slot( self : HashSet[K], - entry : Entry[K], - new_idx : Int, + idx : Int, + psl : Int, + hash : Int, + key : K, ) -> Unit { - self.entries[new_idx] = Some(entry) + self.psls[idx] = psl + self.hashes[idx] = hash + self.keys[idx] = key } ///| @@ -158,11 +161,12 @@ pub fn[K : Hash + Eq] HashSet::contains(self : HashSet[K], key : K) -> Bool { // inline lookup to avoid unnecessary allocations let hash = Hash::hash(key) for i = 0, idx = hash & self.capacity_mask { - guard self.entries[idx] is Some(entry) else { break false } - if entry.hash == hash && entry.key == key { + let psl = self.psls[idx] + guard psl != empty_psl else { break false } + if self.hashes[idx] == hash && self.keys[idx] == key { break true } - if i > entry.psl { + if i > psl { break false } continue i + 1, (idx + 1) & self.capacity_mask @@ -193,13 +197,14 @@ pub fn[K : Hash + Eq] HashSet::contains(self : HashSet[K], key : K) -> Bool { pub fn[K : Hash + Eq] HashSet::remove(self : HashSet[K], key : K) -> Unit { let hash = Hash::hash(key) for i = 0, idx = hash & self.capacity_mask { - guard self.entries[idx] is Some(entry) else { break } - if entry.hash == hash && entry.key == key { + let psl = self.psls[idx] + guard psl != empty_psl else { break } + if self.hashes[idx] == hash && self.keys[idx] == key { self.shift_back(idx) self.size -= 1 break } - if i > entry.psl { + if i > psl { break } continue i + 1, (idx + 1) & self.capacity_mask @@ -210,16 +215,14 @@ pub fn[K : Hash + Eq] HashSet::remove(self : HashSet[K], key : K) -> Unit { fn[K] HashSet::shift_back(self : HashSet[K], idx : Int) -> Unit { for cur = idx { let next = (cur + 1) & self.capacity_mask - match self.entries[next] { - None | Some({ psl: 0, .. }) => { - self.entries[cur] = None - break - } - Some(entry) => { - entry.psl -= 1 - self.set_entry(entry, cur) - continue next - } + let next_psl = self.psls[next] + if next_psl == empty_psl || next_psl == 0 { + self.psls[cur] = empty_psl + set_null(self.keys, cur) + break + } else { + self.set_slot(cur, next_psl - 1, self.hashes[next], self.keys[next]) + continue next } } } @@ -232,42 +235,46 @@ fn[K] HashSet::grow(self : HashSet[K]) -> Unit { self.capacity_mask = self.capacity - 1 self.grow_at = calc_grow_threshold(self.capacity) self.size = 0 - self.entries = FixedArray::make(self.capacity, None) + self.psls = FixedArray::make(self.capacity, empty_psl) + self.hashes = FixedArray::make(self.capacity, 0) + self.keys = UninitializedArray::make(self.capacity) return } - let old_entries = self.entries + let old_psls = self.psls + let old_hashes = self.hashes + let old_keys = self.keys + let old_capacity = self.capacity let new_capacity = self.capacity * 2 - self.entries = FixedArray::make(new_capacity, None) + self.psls = FixedArray::make(new_capacity, empty_psl) + self.hashes = FixedArray::make(new_capacity, 0) + self.keys = UninitializedArray::make(new_capacity) self.capacity = new_capacity self.capacity_mask = new_capacity - 1 self.grow_at = calc_grow_threshold(self.capacity) - for entry in old_entries { - if entry is Some(entry) { - self.rehash_place_entry(entry) + for i in 0.. Unit { - let hash = entry.hash +fn[K] HashSet::rehash_place_entry( + self : HashSet[K], + hash : Int, + key : K, +) -> Unit { for psl = 0, idx = hash & self.capacity_mask { - match self.entries[idx] { - None => { - entry.psl = psl - self.set_entry(entry, idx) - return - } - Some(curr) => - if psl > curr.psl { - self.push_away(idx, curr) - entry.psl = psl - self.set_entry(entry, idx) - return - } else { - continue psl + 1, (idx + 1) & self.capacity_mask - } + let curr_psl = self.psls[idx] + if curr_psl == empty_psl { + self.set_slot(idx, psl, hash, key) + return + } else if psl > curr_psl { + self.push_away(idx, curr_psl, self.hashes[idx], self.keys[idx]) + self.set_slot(idx, psl, hash, key) + return + } else { + continue psl + 1, (idx + 1) & self.capacity_mask } } } @@ -309,9 +316,9 @@ pub fn[K] HashSet::each( self : HashSet[K], f : (K) -> Unit raise?, ) -> Unit raise? { - for entry in self.entries { - if entry is Some({ key, .. }) { - f(key) + for i in 0.. Unit raise?, ) -> Unit raise? { for i in 0.. Unit { - self.entries.fill(None) + // Reuse the existing buffers (keeps the allocated space) and only null the + // occupied key slots so their references can be reclaimed. + for i in 0.. Unit { #alias(iterator, deprecated) pub fn[K] HashSet::iter(self : HashSet[K]) -> Iter[K] { let mut i = 0 - let len = self.entries.length() + let len = self.capacity Iter::new( fn() { while i < len { - let entry = self.entries.unsafe_get(i) + let idx = i i += 1 - if entry is Some({ key, .. }) { - return Some(key) + if self.psls[idx] != empty_psl { + return Some(self.keys[idx]) } } nobreak { None @@ -365,9 +379,13 @@ pub fn[K] HashSet::iter(self : HashSet[K]) -> Iter[K] { ///| /// Converts the hash set to an array. pub fn[K] HashSet::to_array(self : HashSet[K]) -> Array[K] { - [ - for entry in self.entries if entry is Some({ key, .. }) => key - ] + let arr = Array::new(capacity=self.size) + for i in 0.. Bool) -> Unit { let size = self.size let mut j = 0 for i = 0; j < size; i = i + 1 { - while self.entries[i] is Some(entry) { + while self.psls[i] != empty_psl { j += 1 - if f(entry.key) { + if f(self.keys[i]) { break } else { self.shift_back(i) @@ -514,11 +532,12 @@ fn calc_grow_threshold(capacity : Int) -> Int { ///| fn[K : Show] HashSet::_debug_entries(self : HashSet[K]) -> String { - for i in 0.. 0 { s + "," } else { s } - continue match self.entries[i] { - None => s + "_" - Some({ psl, key, .. }) => s + "(\{psl},\{key})" + continue if self.psls[i] == empty_psl { + s + "_" + } else { + s + "(\{self.psls[i]},\{self.keys[i]})" } } nobreak { s @@ -526,7 +545,7 @@ fn[K : Show] HashSet::_debug_entries(self : HashSet[K]) -> String { } ///| -priv struct MyString(String) derive(Eq, @debug.Debug) +priv struct MyString(String) derive(Eq) ///| impl Hash for MyString with fn hash(self) { @@ -628,8 +647,8 @@ test "clear" { m.clear() inspect(m.length(), content="0") inspect(m.capacity(), content="8") - for entry in m.entries { - @test.assert_same_object(entry, None) + for i in 0.. HashSet[K] { let other = { capacity: self.capacity, - entries: FixedArray::make(self.capacity, None), + psls: FixedArray::make(self.capacity, empty_psl), + hashes: FixedArray::make(self.capacity, 0), + keys: UninitializedArray::make(self.capacity), size: self.size, capacity_mask: self.capacity_mask, grow_at: self.grow_at, } - self.entries.blit_to(other.entries, len=self.capacity) + self.psls.blit_to(other.psls, len=self.capacity) + self.hashes.blit_to(other.hashes, len=self.capacity) + UninitializedArray::unsafe_blit(other.keys, 0, self.keys, 0, self.capacity) other } ///| /// ToJson implementation for hashset pub impl[X : ToJson] ToJson for HashSet[X] with fn to_json(self) { - [ - for entry in self.entries if entry is Some({ key, .. }) => key - ] + self.to_array().to_json() } ///| diff --git a/hashset/hashset_bench_test.mbt b/hashset/hashset_bench_test.mbt new file mode 100644 index 0000000000..dc90e2cd2d --- /dev/null +++ b/hashset/hashset_bench_test.mbt @@ -0,0 +1,44 @@ +// Copyright 2026 International Digital Economy Academy +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +///| +let hashset_bench_n = 50000 + +///| +test "bench HashSet::add n=50000" (it : @bench.T) { + it.bench(fn() { + let s = @hashset.HashSet([]) + for i in 0..= 0`, so `-1` is a safe sentinel. +let empty_psl : Int = -1 -/// A mutable hash set implements with Robin Hood hashing. +///| +/// Releases the key reference stored at `idx` so the collector can reclaim it. +fn[K] set_null(keys : UninitializedArray[K], idx : Int) = "%fixedarray.set_null" + +/// A mutable hash set implemented with Robin Hood hashing. /// /// reference: /// - @@ -29,6 +30,12 @@ priv struct Entry[K] { ///| /// Mutable hash set, not thread safe. /// +/// The table is stored as a struct-of-arrays (`psls` / `hashes` / `keys`) +/// rather than an array of boxed entries, so inserting a key does not allocate +/// a per-entry object. For an occupied slot `i`, `hashes[i]` caches the key's +/// hash and `keys[i]` holds the key; for an empty slot `psls[i] == empty_psl` +/// and `keys[i]` is unused. +/// /// # Example /// /// ```mbt check @@ -39,7 +46,9 @@ priv struct Entry[K] { /// } /// ``` struct HashSet[K] { - mut entries : FixedArray[Entry[K]?] + mut psls : FixedArray[Int] // probe sequence length, or empty_psl when empty + mut hashes : FixedArray[Int] // cached key hash for occupied slots + mut keys : UninitializedArray[K] // key storage for occupied slots mut size : Int // active key count mut capacity : Int // current capacity mut capacity_mask : Int // capacity_mask = capacity - 1, used to find idx From 38fbfc8878c74fa90562e0a093c6d345a315654e Mon Sep 17 00:00:00 2001 From: mizchi Date: Sun, 28 Jun 2026 23:50:18 +0900 Subject: [PATCH 2/6] Test HashSet clear buffer reuse --- hashset/hashset.mbt | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/hashset/hashset.mbt b/hashset/hashset.mbt index 070a25636d..6f75480184 100644 --- a/hashset/hashset.mbt +++ b/hashset/hashset.mbt @@ -644,12 +644,30 @@ test "grow" { ///| test "clear" { let m : HashSet[MyString] = new_hashset(default_init_capacity) + fn i(s) { + MyString::MyString(s) + } + + m.add("C" |> i) + m.add("Go" |> i) + m.add("C++" |> i) + m.add("Java" |> i) + let psls = m.psls + let hashes = m.hashes + let keys = m.keys + inspect(m.length(), content="4") m.clear() inspect(m.length(), content="0") inspect(m.capacity(), content="8") + inspect(physical_equal(m.psls, psls), content="true") + inspect(physical_equal(m.hashes, hashes), content="true") + inspect(physical_equal(m.keys, keys), content="true") for i in 0.. i) + inspect(m.length(), content="1") + inspect(m.contains("MoonBit" |> i), content="true") } ///| From 5fdfe805f151849b806e5c48d4d618c4f288b973 Mon Sep 17 00:00:00 2001 From: mizchi Date: Mon, 6 Jul 2026 12:18:58 +0900 Subject: [PATCH 3/6] Fix HashSet copy of sparse key storage --- hashset/hashset.mbt | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/hashset/hashset.mbt b/hashset/hashset.mbt index 6f75480184..72a6b4861b 100644 --- a/hashset/hashset.mbt +++ b/hashset/hashset.mbt @@ -670,6 +670,40 @@ test "clear" { inspect(m.contains("MoonBit" |> i), content="true") } +///| +test "copy sparse set only copies occupied key slots" { + let m : HashSet[MyString] = new_hashset(default_init_capacity) + fn i(s) { + MyString::MyString(s) + } + + m.add("a" |> i) + m.add("ab" |> i) + m.add("bc" |> i) + m.add("cd" |> i) + m.add("abc" |> i) + m.add("abcdef" |> i) + m.remove("ab" |> i) + let copied = m.copy() + inspect(copied.length(), content="5") + inspect(copied.capacity(), content="8") + inspect(physical_equal(copied.psls, m.psls), content="false") + inspect(physical_equal(copied.hashes, m.hashes), content="false") + inspect(physical_equal(copied.keys, m.keys), content="false") + let mut occupied = 0 + for idx in 0.. i), content="false") + inspect(copied.contains("abcdef" |> i), content="true") +} + ///| /// Insert a key into the hash set and returns whether the key was successfully added. /// @@ -745,7 +779,11 @@ pub fn[K] HashSet::copy(self : HashSet[K]) -> HashSet[K] { } self.psls.blit_to(other.psls, len=self.capacity) self.hashes.blit_to(other.hashes, len=self.capacity) - UninitializedArray::unsafe_blit(other.keys, 0, self.keys, 0, self.capacity) + for i in 0.. Date: Mon, 27 Jul 2026 19:13:03 +0900 Subject: [PATCH 4/6] perf(hashset): avoid bounds checks in contains --- hashset/hashset.mbt | 29 +++++++++++++++++++++++++++-- hashset/types.mbt | 7 +++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/hashset/hashset.mbt b/hashset/hashset.mbt index 72a6b4861b..39f2e3bee7 100644 --- a/hashset/hashset.mbt +++ b/hashset/hashset.mbt @@ -161,9 +161,14 @@ pub fn[K : Hash + Eq] HashSet::contains(self : HashSet[K], key : K) -> Bool { // inline lookup to avoid unnecessary allocations let hash = Hash::hash(key) for i = 0, idx = hash & self.capacity_mask { - let psl = self.psls[idx] + // SAFETY: `idx` is masked by `capacity_mask`; all backing arrays have + // length `capacity`, so the probe index is in bounds. + let psl = self.psls.unsafe_get(idx) guard psl != empty_psl else { break false } - if self.hashes[idx] == hash && self.keys[idx] == key { + // SAFETY: the same index invariant applies to `hashes`; a non-empty PSL + // also guarantees that `keys[idx]` was initialized by `set_slot`. + if self.hashes.unsafe_get(idx) == hash && + unsafe_get_initialized_key(self.keys, idx) == key { break true } if i > psl { @@ -559,6 +564,26 @@ impl Hash for MyString with fn hash_combine(self, hasher) { hasher.combine_string(self) } +///| +test "contains handles collided slots across table wraparound" { + let m : HashSet[MyString] = new_hashset(8) + fn key(value) { + MyString::MyString(value) + } + + // All keys hash to 7, so the probe sequence starts at the final slot and + // wraps around to the start of the table. + m.add("aaaaaaa" |> key) + m.add("bbbbbbb" |> key) + m.add("ccccccc" |> key) + m.add("ddddddd" |> key) + m.add("eeeeeee" |> key) + + assert_true(m.contains("aaaaaaa" |> key)) + assert_true(m.contains("eeeeeee" |> key)) + assert_false(m.contains("fffffff" |> key)) +} + ///| test "set" { let m : HashSet[MyString] = new_hashset(default_init_capacity) diff --git a/hashset/types.mbt b/hashset/types.mbt index 87fa57cec4..c2087127f2 100644 --- a/hashset/types.mbt +++ b/hashset/types.mbt @@ -21,6 +21,13 @@ let empty_psl : Int = -1 /// Releases the key reference stored at `idx` so the collector can reclaim it. fn[K] set_null(keys : UninitializedArray[K], idx : Int) = "%fixedarray.set_null" +///| +/// Reads an initialized key slot without checking its index. +/// +/// Callers must ensure that `idx` is within the backing array and refers to an +/// occupied slot. +fn[K] unsafe_get_initialized_key(keys : UninitializedArray[K], idx : Int) -> K = "%fixedarray.unsafe_get" + /// A mutable hash set implemented with Robin Hood hashing. /// /// reference: From 855e773f179f112936a661f54d7511f0c1c3fda7 Mon Sep 17 00:00:00 2001 From: Hongbo Zhang Date: Sat, 22 Aug 2026 10:01:19 +0800 Subject: [PATCH 5/6] perf(hashset): use unchecked access on the insertion hot path The struct-of-arrays layout replaces one checked array access per probe step with three, and on js -- where the malloc and reference-counting costs the layout removes do not exist -- those checks made `add` about 28% slower than the boxed layout it replaces. `contains` already probes with `unsafe_get` under a documented index invariant. The same invariant holds everywhere else the table is probed: the index starts as `hash & capacity_mask` and every step re-masks it, so it is in bounds for all three arrays, and a non-empty PSL guarantees the key slot was initialized. Apply it to `add_with_hash`, `push_away`, `rehash_place_entry`, `grow` and `set_slot`, each with the reasoning written out. `add`, n=50000, against the boxed layout on main: | backend | main | SoA | 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 returns to parity and native gains a further 15% on top of the layout change, for 38% against main overall. `contains` is unchanged by this commit on every backend. Co-Authored-By: Claude Opus 5 (1M context) --- hashset/hashset.mbt | 50 +++++++++++++++++++++++++++++++++------------ hashset/types.mbt | 3 +++ 2 files changed, 40 insertions(+), 13 deletions(-) diff --git a/hashset/hashset.mbt b/hashset/hashset.mbt index 39f2e3bee7..5a019dd61e 100644 --- a/hashset/hashset.mbt +++ b/hashset/hashset.mbt @@ -96,16 +96,26 @@ fn[K : Eq] HashSet::add_with_hash( self.grow() } let (idx, psl) = for psl = 0, idx = hash & self.capacity_mask { - let curr_psl = self.psls[idx] + // SAFETY: `idx` starts masked by `capacity_mask` and every step re-masks + // it, so it indexes all three backing arrays -- each of length + // `capacity` -- in bounds. A non-empty PSL further guarantees that + // `keys[idx]` was initialized by `set_slot`. + let curr_psl = self.psls.unsafe_get(idx) if curr_psl == empty_psl { break (idx, psl) } - if self.hashes[idx] == hash && self.keys[idx] == key { + if self.hashes.unsafe_get(idx) == hash && + unsafe_get_initialized_key(self.keys, idx) == key { return } if psl > curr_psl { // Displace the richer occupant of `idx`, then write the new key here. - self.push_away(idx, curr_psl, self.hashes[idx], self.keys[idx]) + self.push_away( + idx, + curr_psl, + self.hashes.unsafe_get(idx), + unsafe_get_initialized_key(self.keys, idx), + ) break (idx, psl) } continue psl + 1, (idx + 1) & self.capacity_mask @@ -126,13 +136,14 @@ fn[K] HashSet::push_away( key : K, ) -> Unit { for psl = psl + 1, idx = (idx + 1) & self.capacity_mask, hash = hash, key = key { - let curr_psl = self.psls[idx] + // SAFETY: same masked-index and initialized-key invariants as `add_with_hash`. + let curr_psl = self.psls.unsafe_get(idx) if curr_psl == empty_psl { self.set_slot(idx, psl, hash, key) break } else if psl > curr_psl { - let curr_hash = self.hashes[idx] - let curr_key = self.keys[idx] + let curr_hash = self.hashes.unsafe_get(idx) + let curr_key = unsafe_get_initialized_key(self.keys, idx) self.set_slot(idx, psl, hash, key) continue curr_psl + 1, (idx + 1) & self.capacity_mask, curr_hash, curr_key } else { @@ -150,9 +161,11 @@ fn[K] HashSet::set_slot( hash : Int, key : K, ) -> Unit { - self.psls[idx] = psl - self.hashes[idx] = hash - self.keys[idx] = key + // SAFETY: every caller derives `idx` from a `capacity_mask` probe, so it is + // in bounds for all three arrays. + self.psls.unsafe_set(idx, psl) + self.hashes.unsafe_set(idx, hash) + unsafe_set_key(self.keys, idx, key) } ///| @@ -257,8 +270,13 @@ fn[K] HashSet::grow(self : HashSet[K]) -> Unit { self.capacity_mask = new_capacity - 1 self.grow_at = calc_grow_threshold(self.capacity) for i in 0.. Unit { for psl = 0, idx = hash & self.capacity_mask { - let curr_psl = self.psls[idx] + // SAFETY: same masked-index and initialized-key invariants as `add_with_hash`. + let curr_psl = self.psls.unsafe_get(idx) if curr_psl == empty_psl { self.set_slot(idx, psl, hash, key) return } else if psl > curr_psl { - self.push_away(idx, curr_psl, self.hashes[idx], self.keys[idx]) + self.push_away( + idx, + curr_psl, + self.hashes.unsafe_get(idx), + unsafe_get_initialized_key(self.keys, idx), + ) self.set_slot(idx, psl, hash, key) return } else { diff --git a/hashset/types.mbt b/hashset/types.mbt index c2087127f2..8f31f12f61 100644 --- a/hashset/types.mbt +++ b/hashset/types.mbt @@ -28,6 +28,9 @@ fn[K] set_null(keys : UninitializedArray[K], idx : Int) = "%fixedarray.set_null" /// occupied slot. fn[K] unsafe_get_initialized_key(keys : UninitializedArray[K], idx : Int) -> K = "%fixedarray.unsafe_get" +///| +fn[K] unsafe_set_key(keys : UninitializedArray[K], idx : Int, key : K) -> Unit = "%fixedarray.unsafe_set" + /// A mutable hash set implemented with Robin Hood hashing. /// /// reference: From 8f58ca82511ec37772562987507dd60e18721d14 Mon Sep 17 00:00:00 2001 From: Hongbo Zhang Date: Sat, 22 Aug 2026 10:17:28 +0800 Subject: [PATCH 6/6] refactor(hashset): declare the unchecked key accessors as methods The unchecked key accessors were free functions taking the array as their first argument, which read differently from the `FixedArray` accessors beside them. Declare them as methods on `UninitializedArray` instead, so every probe site reads uniformly: self.keys.unsafe_get(idx) self.keys.unsafe_set(idx, key) self.keys.set_null(cur) `builtin` already has package-private versions of all three, but they stay private there deliberately: `unsafe_set` and `set_null` can corrupt memory or resurrect a freed slot, so the fewer packages that can name them, the better. These declarations are package-local to `hashset` and go no further. Also note at `shift_back`'s `set_null` why it is not redundant with the `psls` write above it -- that is the one place the call looks removable, and removing it would leave every test passing while retaining up to one dead key per vacated slot. Co-Authored-By: Claude Opus 5 (1M context) --- hashset/hashset.mbt | 26 ++++++++++++-------------- hashset/types.mbt | 26 ++++++++++++++++++-------- 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/hashset/hashset.mbt b/hashset/hashset.mbt index 5a019dd61e..0a1ca9136b 100644 --- a/hashset/hashset.mbt +++ b/hashset/hashset.mbt @@ -104,8 +104,7 @@ fn[K : Eq] HashSet::add_with_hash( if curr_psl == empty_psl { break (idx, psl) } - if self.hashes.unsafe_get(idx) == hash && - unsafe_get_initialized_key(self.keys, idx) == key { + if self.hashes.unsafe_get(idx) == hash && self.keys.unsafe_get(idx) == key { return } if psl > curr_psl { @@ -114,7 +113,7 @@ fn[K : Eq] HashSet::add_with_hash( idx, curr_psl, self.hashes.unsafe_get(idx), - unsafe_get_initialized_key(self.keys, idx), + self.keys.unsafe_get(idx), ) break (idx, psl) } @@ -143,7 +142,7 @@ fn[K] HashSet::push_away( break } else if psl > curr_psl { let curr_hash = self.hashes.unsafe_get(idx) - let curr_key = unsafe_get_initialized_key(self.keys, idx) + let curr_key = self.keys.unsafe_get(idx) self.set_slot(idx, psl, hash, key) continue curr_psl + 1, (idx + 1) & self.capacity_mask, curr_hash, curr_key } else { @@ -165,7 +164,7 @@ fn[K] HashSet::set_slot( // in bounds for all three arrays. self.psls.unsafe_set(idx, psl) self.hashes.unsafe_set(idx, hash) - unsafe_set_key(self.keys, idx, key) + self.keys.unsafe_set(idx, key) } ///| @@ -180,8 +179,7 @@ pub fn[K : Hash + Eq] HashSet::contains(self : HashSet[K], key : K) -> Bool { guard psl != empty_psl else { break false } // SAFETY: the same index invariant applies to `hashes`; a non-empty PSL // also guarantees that `keys[idx]` was initialized by `set_slot`. - if self.hashes.unsafe_get(idx) == hash && - unsafe_get_initialized_key(self.keys, idx) == key { + if self.hashes.unsafe_get(idx) == hash && self.keys.unsafe_get(idx) == key { break true } if i > psl { @@ -236,7 +234,10 @@ fn[K] HashSet::shift_back(self : HashSet[K], idx : Int) -> Unit { let next_psl = self.psls[next] if next_psl == empty_psl || next_psl == 0 { self.psls[cur] = empty_psl - set_null(self.keys, cur) + // 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) break } else { self.set_slot(cur, next_psl - 1, self.hashes[next], self.keys[next]) @@ -273,10 +274,7 @@ fn[K] HashSet::grow(self : HashSet[K]) -> Unit { // SAFETY: `i < old_capacity`, the length of all three old arrays, and a // non-empty PSL means the old key slot was initialized. if old_psls.unsafe_get(i) != empty_psl { - self.rehash_place_entry( - old_hashes.unsafe_get(i), - unsafe_get_initialized_key(old_keys, i), - ) + self.rehash_place_entry(old_hashes.unsafe_get(i), old_keys.unsafe_get(i)) } } } @@ -298,7 +296,7 @@ fn[K] HashSet::rehash_place_entry( idx, curr_psl, self.hashes.unsafe_get(idx), - unsafe_get_initialized_key(self.keys, idx), + self.keys.unsafe_get(idx), ) self.set_slot(idx, psl, hash, key) return @@ -377,7 +375,7 @@ pub fn[K] HashSet::clear(self : HashSet[K]) -> Unit { for i in 0.. K = "%fixedarray.unsafe_get" ///| -/// Reads an initialized key slot without checking its index. -/// -/// Callers must ensure that `idx` is within the backing array and refers to an -/// occupied slot. -fn[K] unsafe_get_initialized_key(keys : 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] unsafe_set_key(keys : UninitializedArray[K], idx : Int, key : K) -> Unit = "%fixedarray.unsafe_set" +/// Releases the key reference stored at `idx` so the collector can reclaim it. +fn[K] UninitializedArray::set_null( + self : UninitializedArray[K], + idx : Int, +) -> Unit = "%fixedarray.set_null" /// A mutable hash set implemented with Robin Hood hashing. ///