perf(builtin): derive Map's probe-sequence length instead of storing it

[bobzhang]

Entry carried a mut psl : Int alongside hash, key and value. That field is not independent state — an entry's probe-sequence length is the distance between the slot it occupies and the slot its hash asked for:

psl = (idx - (hash & capacity_mask)) & capacity_mask

Every write to it happened as the entry landed in a slot and always equalled that displacement; every read happens inside a probe loop that already knows the slot index. So psl_at computes it where needed and the field goes away — five fields per entry instead of six.

shift_back no longer decrements anything: moving an entry one slot back lowers its derived PSL by exactly one. push_away and rehash_place_entry likewise stop writing PSLs and just place entries.

No layout change. The table is still FixedArray[Entry[K, V]?], so none of the struct-of-arrays tradeoffs from #4127 / #4131 apply here. Two files, net −4 lines.

Checking the premise first

psl being mut suggests independent state, so I verified rather than argued. Instrumenting set_entry and add_entry_to_tail — every path that places an entry in a slot — to assert psl == (idx - (hash & mask)) & mask on each write, the full suite passes with that assertion live on all three backends: 7544 native, 7485 js, 7544 wasm-gc, no violation.

Measurements

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

backend	op	main	this branch	change
js	set	3.04 ms	2.90 ms	5% faster
js	get hit	1.01 ms	1.00 ms	unchanged
js	get miss	1.16 ms	1.15 ms	unchanged
js	set+remove	4.30 ms	4.29 ms	unchanged
native	set	2.62 ms	2.63 ms	unchanged
native	get hit	855 µs	832 µs	3% faster
native	get miss	1.01 ms	1.02 ms	unchanged
native	set+remove	3.82 ms	3.73 ms	2% faster
wasm-gc	set	2.46 ms	2.42 ms	2% faster
wasm-gc	get hit	1.07 ms	1.07 ms	unchanged
wasm-gc	get miss	1.19 ms	1.20 ms	unchanged

Insertion gains where the smaller entry matters most; lookups are flat, which fits — the derivation costs two arithmetic operations per probe step against a field read from an object already in cache. Nothing regresses.

I have left the Map::each figures out: its wasm-gc timing varied between 57 µs, 190 µs and 64 µs across runs on a path this change does not touch, so I do not trust it either way.

Provenance

The derivation is Codex CLI's, proposed while explaining the js regression on #4127. Its application to the boxed layout — where it needs no layout change at all — came out of verifying that the identity held independently of struct-of-arrays.

Codex CLI verification at xhigh is running; its verdict will be posted as a comment.

[Copilot]

Pull request overview

This PR optimizes builtin’s linked_hash_map by removing stored probe-sequence-length (PSL) state from Entry and deriving PSL on demand via Map::psl_at, reducing per-entry fields while preserving the existing boxed table layout.

Changes:

• Removed Entry.psl and introduced Map::psl_at(idx, entry) to compute PSL from (idx - (hash & mask)) & mask.
• Updated probe/insert/remove/grow logic (set_with_hash, push_away, get*, shift_back, rehash_place_entry, etc.) to use derived PSL.
• Updated whitebox tests/debug helpers to compute PSL via psl_at.

Reviewed changes

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

File	Description
builtin/linked_hash_map.mbt	Removes stored PSL from Entry and updates Robin Hood probing logic to derive PSL via psl_at.
builtin/linked_hash_map_wbtest.mbt	Updates tests and debug formatting to use derived PSL instead of the removed Entry.psl field.

Suppressed comments (2)

builtin/linked_hash_map.mbt:421

• This loop still returns psl in the break tuple, but the binding discards it (_). Since PSL isn’t stored anymore, the tuple can be simplified to avoid carrying an unused value through the control flow.

  let (idx, _, push_away) = for psl = 0, idx = hash & self.capacity_mask {

builtin/linked_hash_map.mbt:1144

• Map::update still returns psl in the break tuple (break (idx, psl, ...)) even though the binding now discards it (let (idx, _, ...)). Since PSL is derived, this extra tuple element is dead and makes the control flow noisier; consider simplifying the tuple to remove psl (and updating the destructuring and both break sites accordingly).

  let (idx, _, new_value, push_away) = for psl = 0, idx = hash &
                                             self.capacity_mask {
    match self.entries.unsafe_get(idx) {
      Some(entry) => {
        if entry.hash == hash && entry.key == key {

---

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

[coveralls]

Coverage Report for CI Build 6302

Coverage increased (+0.006%) to 90.883%

Details

• Coverage increased (+0.006%) from the base build.
• Patch coverage: 22 of 22 lines across 1 file are fully covered (100%).
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	18460
Covered Lines:	16777
Line Coverage:	90.88%
Coverage Strength:	305894.13 hits per line

---

💛 - Coveralls

[bobzhang]

Codex CLI verification (xhigh) — changes requested

It ran the benchmarks this time (my earlier reviews used a read-only sandbox, which is why it had been refusing to certify numbers) and found a regression my own measurements missed.

Verdict: I would not land this as-is. The correctness review passed, but the requested remeasurement found a repeatable miss-lookup regression.

• Map::get misses became approximately 3–4% slower on native and wasm-gc. Four additional targeted ABBA rounds confirmed it; this is consistent with deriving PSL during every unsuccessful probe.
• The assertion at linked_hash_map_wbtest.mbt:588 is vacuous: it passes the entry's ideal index to psl_at, which necessarily returns zero. It should pass the actual slot, likely map.tail in this test.

Backend	Operation	Base	PR	Change
native	set	2.52 ms	2.57 ms	2.2% slower
native	get hit	0.830 ms	0.835 ms	0.6% slower
native	get miss	0.973 ms	1.02 ms	4.8% slower
JS	set	3.09 ms	2.89 ms	6.5% faster
JS	set+remove	4.40 ms	4.27 ms	2.8% faster
wasm-gc	set	2.43 ms	2.40 ms	1.2% faster
wasm-gc	get hit	1.08 ms	1.06 ms	1.4% faster
wasm-gc	get miss	1.16 ms	1.20 ms	3.4% slower

Correctness itself looks sound: every production psl_at call uses the slot from which that entry was read; push_away preserves the Robin Hood recurrence and computing displaced PSL before the store is correct; shift_back is equivalent across wraparound; rehashing reconstructs placement correctly, and copy preserves capacity and slot indices.

Both findings were right and both are fixed.

The vacuous assertion was a real bug in my test — I passed head.hash & map.capacity_mask, the entry's ideal index, where psl_at returns 0 by construction. It now passes map.tail, the actual slot, with a comment saying why.

The miss regression I should have predicted. My own reasoning on #4126 was that misses gain most from removing per-probe work because they walk the sequence to its end — the same logic says they lose most from adding it. Hoisting capacity_mask into a local in get's probe loop (deriving a PSL reads it twice more per step than the stored field did) halves the cost, confirmed over two paired rounds: native miss goes from 4.8% to about 2.2%. It does not remove it.

So the honest position, now in the commit message: this is a trade, not a free win.

operation	js	native	wasm-gc
set	6.5% faster	unchanged	1.2% faster
set+remove	2.8% faster	unchanged	unchanged
get hit	unchanged	2.7% faster	1.4% faster
get miss	unchanged	~2% slower	~3% slower

Whether that is worth taking depends on the workload mix — miss-heavy lookups are common in JSON parsing and contains checks. I would rather put the tradeoff in front of a maintainer than land it as though it were free.