Add a hash map which keeps its keys as they are - #468
Conversation
HashMap casts every key to an int and stores it in a Table. That works for handles and for anything castable, but it loses the type on the way in: two keys which cast to the same int collide, and a key which is not castable cannot be used at all. FastHashMap takes a bound on its key type instead, so hashing and comparing are done by the key's own implementation and the key is stored as itself. Instances for int and string come with the package; declaring one beside your own type is what makes it usable as a key. Storage is one array per specialisation carved into a section per instance, as ArrayList does it. A section is a fixed number of slots and does not grow, so a map which fills up refuses further keys rather than rehashing, and both the section size and the number of sections are configurable. Collisions probe linearly inside the section; a removed slot becomes a tombstone rather than empty, so a probe which passed over it still finds keys put down beyond it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76b6d9ba72
ℹ️ 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".
| and never reclaimed, so this is a total over the run rather than a live count. */ | ||
| @configurable public constant FASTHASHMAP_MAX_INSTANCES = 256 | ||
|
|
||
| constant SLOTS = FASTHASHMAP_CAPACITY * FASTHASHMAP_MAX_INSTANCES |
There was a problem hiding this comment.
Bound configured storage to the JASS array limit
When FASTHASHMAP_CAPACITY * FASTHASHMAP_MAX_INSTANCES exceeds JASS_MAX_ARRAY_SIZE—for example, after following the API documentation's advice to raise the capacity while retaining 256 instances—the allocator accepts sections whose indices exceed the fixed JASS array bounds. Those maps then access invalid slots and cannot reliably store or retrieve entries; unlike ArrayList.allocateStorage, there is no native-target limit check. Validate the configuration or shard the storage before allocating such sections.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
| used[s] = false | ||
| dead[s] = true |
There was a problem hiding this comment.
Release removed entries on the Lua target
On Lua, removing an entry only changes the occupancy flags, while the static keys and values arrays continue referencing the removed key and value; clear() similarly leaves every stored reference intact. Maps containing temporary strings or other collectible values therefore retain the last occupant of every used slot for the lifetime of the specialization, even though the entries are logically gone. Null the vacated key/value slots on Lua in both remove() and clear() so the garbage collector can reclaim them.
Useful? React with 👍 / 👎.
Three things, the first of which the build caught. The overview comment sat at package level with another doc comment after it, so it documented nothing and hotdoc rejected the position. It belongs on the class it describes, which is where LinkedList keeps its own. The section count was checked against the configured total and not against the array holding it. Raising FASTHASHMAP_CAPACITY while leaving FASTHASHMAP_MAX_INSTANCES alone - which the overview suggested doing - can put that total past JASS_MAX_ARRAY_SIZE, and sections were then handed out past the end of the array on a target where it cannot grow. Guarded as ArrayList.allocateStorage guards it, on the Jass target only, since Lua grows the table. A vacated slot kept the key and value it held. On Lua those are references the map no longer owns, so a removed entry and a cleared map held their last occupant for the lifetime of the specialisation. Released in both remove and clear, behind the same isLua branch ArrayList uses for the same reason; a tombstone is never read for its key, so this is safe. The overview also named FastHashMap_CAPACITY, which is not the constant.
Wurst re-exports MagicFunctions, which is where isLua comes from, but not ErrorHandling, so the two error() calls guarding a failed allocation resolved to nothing. The earlier hotdoc failure stopped compilation before name resolution ran, which is why this only surfaced once that was fixed. Verified against the whole library this time rather than pushed to find out: 474/474, including this package's fourteen.
|
CI is green now. Three rounds, and both review findings are in. Build failure (mine). The overview comment sat at package level with another doc comment after it, so it documented nothing and hotdoc rejected the position. Moved onto the class it describes, which is where P1, storage past the array bound — accepted. The section count was checked against the configured total and not against the array holding it, so raising P2, retention on Lua — accepted. A vacated slot kept the key and value it held, so a removed entry and a cleared map held their last occupant for the lifetime of the specialisation. Released in both Verified against the whole library rather than pushed to find out this time: @codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Both instances leaned on something that collides on ordinary keys, and a section is FASTHASHMAP_CAPACITY slots wide, so a map whose keys collide refuses them once it fills rather than merely probing longer. StringHash is case insensitive, so alpha and ALPHA had one hash, and it collapses every partial multibyte slice to a single constant. The string hash now mixes each byte with its position and the length, so anagrams and prefixes separate too. Single bytes are still decoded through StringUtils.char, which recovers the case StringHash loses and which the library already depends on throughout; non-latin text stays the weak case, its lead bytes not decoding, and says so. The int hash returned the key itself, which sends everything strided by the capacity to slot zero - ids, handles and loop counters all arrive strided like that. Halves are mixed separately so the multiplications stay in range. Every intermediate stays below 2^31 rather than relying on overflow, which wraps at 32 bits on Jass and does not on Lua. Nothing stores a hash, so differing values would not have been wrong, but the interpreter could then no longer stand in for the game. The behavioural tests here pass with the old hashes as well, because probing separates colliding keys whatever they hash to. Reaching the requirement through hashOf is what observes the hash itself: against the old ones those tests fail with all sixteen strided keys in one slot, and with alpha and ALPHA hashing alike.
nextFree only ever grew and nothing was ever released, so FASTHASHMAP_MAX_INSTANCES counted maps ever made rather than maps alive. A map built per spell cast or per unit therefore exhausted the sections and every later one refused its keys with the section limit error - which is the ordinary way a map uses a container, so this had to be fixed before the container is usable in one. Every section is the same width, so released ones are a stack rather than the capacity-matched free list ArrayList keeps: any released section fits any new map, and there is nothing to compact. Emptied on the way out rather than on the way in, so the next map gets a clean section without paying for it and nothing keeps a reference the map no longer owns. ondestroy takes neither a hotdoc comment nor a return, hence the line comments and the inverted guard. The three tests fail without the reuse: creating past the instance limit reports out of sections, and the two checking that a reused section starts empty and that a live map keeps its own were written against that failure first.
|
Two rounds since: a real hash, and the lifecycle gap that would have bitten in an actual map. The hash is computed here now, not taken from Correction to something I wrote earlier in this PR: the interpreter does emulate A destroyed map now hands its section back. On the tests. The first batch I wrote for the hash passed against the old hashes too, because probing separates colliding keys however badly they hash — coverage in name only. Reaching the requirement through a
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0521e2aba1
ℹ️ 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".
|
|
||
| /** Slots per map. Fixed at compile time: every map of one key and value type is this | ||
| size, so raising it costs memory across all of them. */ | ||
| @configurable public constant FASTHASHMAP_CAPACITY = 32 |
There was a problem hiding this comment.
Reject a non-positive configured capacity
When a consumer configures FASTHASHMAP_CAPACITY to zero, construction succeeds because both allocation bounds compare zero against zero, but the first put, get, has, or remove evaluates K.hash(key) mod FASTHASHMAP_CAPACITY and terminates the current thread with division by zero. Since this is an exported configurable value with no documented lower bound, validate that it is positive before handing out a section.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
| /** The number of maps of one key and value type which can exist. Sections are handed out | ||
| and never reclaimed, so this is a total over the run rather than a live count. */ | ||
| @configurable public constant FASTHASHMAP_MAX_INSTANCES = 256 |
There was a problem hiding this comment.
Document the instance limit as a live-map bound
The public configuration comment still says sections are never reclaimed and that the limit counts maps over the entire run, but ondestroy now pushes each section onto freeSection for reuse. Consumers following this documentation may size FASTHASHMAP_MAX_INSTANCES for lifetime allocations rather than simultaneously live maps, unnecessarily increasing storage or hitting the Jass array limit.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
Two things a map needs before it is usable in one. put dropped a new key silently once the section was full. Losing a store without a word is close to impossible to find from the outside - the map simply does not have what you put in it - so it reports now. A key the map already holds stays writable, as before, and isFull is there to ask beforehand. The test which relied on the silent drop asks isFull instead. There was no way to see what a map holds. nextEntry walks to the next occupied slot and keyAt/valueAt read it, which allocates nothing - what a map iterating every frame needs - and avoids dispatching a bound through a closure, which is not supported on every target. A for-in wrapper can be built on top; this is the primitive under it. nextEntry skips tombstones, so a removed key is not visited and its slot does not stop the walk early. keyAt and valueAt report on a slot holding nothing rather than returning whatever the array has there. startSlot rather than from, from being reserved.
|
Both gaps closed. A dropped write reports. A map can be walked. There was no way to see what one holds: Slot walking rather than an iterator object or a closure: it allocates nothing, which is what a map iterating every frame needs, and it avoids dispatching a bound through a closure — a shape that is not supported on every target, as
One naming note for the next reader: the parameter is @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 629095c779
ℹ️ 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".
| function keyAt(int slot) returns K | ||
| if base < 0 or slot < 0 or slot >= FASTHASHMAP_CAPACITY or not used[base + slot] | ||
| error("FastHashMap: keyAt on a slot which holds nothing; use the value nextEntry returned.") | ||
| return keys[base] |
There was a problem hiding this comment.
Return a default key after rejecting an empty slot
On the Lua target, ErrorHandling.error records lastError and returns, so calling keyAt with an empty, removed, or out-of-range slot continues into this fallback and returns keys[base]. That slot may contain an unrelated live key, directly contradicting the API's promise not to hand back arbitrary array contents and potentially causing callers to process the wrong entry; use a never-written K array, analogous to none, for the required fallback value.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
|
Description rewritten — it still described only the original container, and the branch has since grown a custom hash, section reuse on destroy, a reported rather than silent dropped write, and iteration. Six commits is a lot to reconstruct that from. One thing worth stating explicitly for review: this branch predates #467, so a tip-to-tip diff shows @codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Decoding each byte and mixing it cost about two native calls and three string comparisons per character: charAt is a SubString, and StringUtils.char is two comparisons, a StringHash and a round trip through the code table. Forty native calls for a twenty character key, against one for StringHash - not a trade a container called fast should make on every lookup. StringHash does the bytes now, and its two defects are patched at fixed cost instead. Two whole-string comparisons separate the cases which occur in practice, all lower, all upper and mixed, so alpha and ALPHA no longer share a hash; two mixed-case spellings of one word still collide and are separated by probing. Length is mixed in, which separates non-latin keys of different length where the multibyte collapse would give them one raw hash; same-length ones still collide and fall back on equals. Order sensitivity comes from StringHash itself. The tests still hold: against plain StringHash the case test fails.
HashMapcasts every key to an int and stores it in aTable. That works for handles and for anything castable, but it loses the type on the way in: two keys which cast to the same int collide, and a key which is not castable cannot be used at all.FastHashMaptakes a bound on its key type instead, so hashing and comparing are done by the key's own implementation and the key is stored as itself:Instances for
intandstringcome with the package. Declaring one beside your own type is what makes it usable as a key.Storage
One array per specialisation, carved into a section per instance, the way
ArrayListworks. A section isFASTHASHMAP_CAPACITYslots and does not grow; collisions probe linearly inside it, and a removed slot becomes a tombstone rather than empty so a probe which passed over it still finds keys put down beyond it.A destroyed map hands its section back, so
FASTHASHMAP_MAX_INSTANCESbounds maps alive rather than maps ever made. Every section is the same width, so released ones are a stack rather than the capacity-matched free listArrayListkeeps — any released section fits any new map, and there is nothing to compact. On the Jass target the configured total is also checked againstJASS_MAX_ARRAY_SIZE, the storage being one fixed-size array.The hash is computed here
Neither instance takes a shortcut, because a section fills up: colliding keys do not merely probe longer, the map eventually refuses them.
StringHashis unusable for this. It is case insensitive, soalphaandALPHAwould share a slot, and it collapses every partial multibyte slice to one constant. The string hash mixes each byte with its position and the length instead, so anagrams and prefixes separate too. Single bytes still decode throughStringUtils.char, which recovers the caseStringHashloses and which the library already depends on throughout; non-latin text is the remaining weak case, its lead bytes not decoding, and the doc says so.The int hash returned the key itself at first, which sends everything strided by the capacity to slot zero — ids, handles and loop counters all arrive strided like that. Halves are mixed separately so the multiplications stay in range.
Every intermediate stays below 2^31 rather than relying on overflow, which wraps at 32 bits on Jass and does not on Lua. Nothing stores a hash, so differing values would not have been wrong, but the interpreter could then no longer stand in for the game while testing distribution.
Reporting and iteration
A full map reports a dropped write rather than discarding it silently — losing a store without a word is close to impossible to find from the outside. A key the map already holds stays writable, and
isFull()asks beforehand.Entries can be walked without allocating:
Slot walking rather than an iterator object or a closure: nothing is allocated, and it avoids dispatching a bound through a closure, which is not supported on every target. A
for inwrapper can be layered on this.Compiler requirement
Type class bounds for
T:generics, shipped in wurstscript/WurstScript#1226, #1228 and #1229. The bound is what makes the key's ownhashandequalsreachable from inside the generic, and it lowers to a direct call to the instance function after specialisation, on both Jass and Lua.Testing
grill testreports 489/489, of which 25 are this package's. Beyond the obvious behaviour — put/get, replacement, missing keys reading as the value type's default, colliding keys, removal keeping later keys reachable, tombstone reuse, independent instances, two specialisations coexisting — the tests cover the three things that would otherwise pass quietly:hashOf<T: Hashable>helper. The behavioural tests pass with an identity hash and withStringHash, because probing separates colliding keys however badly they hash; only observing the hash catches it. Against the old ones these fail with all sixteen strided keys in one slot and withalphaandALPHAhashing alike.out of sections.Every test here was checked to fail before it passed.
Note
The container is also exercised from the compiler side in
FastHashMapTeststhere — standalone and with the standard library in scope, on both targets — since it is the shape the bounds feature was built for. One gap worth stating: the standard-library-in-scope case is compiled but not executed on Lua, because the test runtime shim cannot initialise the library's own packages.A later step could give Lua a native table instead of this probing, which would lift
FASTHASHMAP_CAPACITYthere; that is wurstscript/WurstScript#1255, and it is deliberately not part of this.