Fix FNV-collision symbol aliasing and memoize OP_SYMBOL#192
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two
Symbol-interning changes, split out from #191 as requested.Bug fix: FNV-1a collision aliases two different symbol names
SymbolTable's dictionary key compared only the 32-bit FNV-1a hash, so two different names with colliding hashes interned to the sameSymbol(~50% probability once ~65k symbols are interned; deterministic for known pairs likecostarring/liquid). The generatedNames.TryFindhad the same flaw for hashes that map to a single known symbol: it returned the known symbol without comparing the name, so a user symbol colliding with e.g.+'s hash would alias it.SymbolTablenow buckets by hash with a per-bucket chain of(name, symbol)entries and always compares the actual bytes.SequenceEqualcheck, including single-entry hash cases.optcarrot is unaffected (18.8 fps, matching main; checksum 59662).
Memoize OP_SYMBOL results per irep
OP_SYMBOLre-interned its pool string (FNV hash + dictionary probe) on every execution. The result is now memoized on theIrep(IrepPoolSymbolCache).Symbols are interned per state, so the cache carries the owning state's id, and the id + symbol array live in one immutable object — a state can never observe another state's symbols through a torn owner/array pair, even if an
Irepis shared between states or threads.Note: the bundled mruby compiler emits
OP_LOADSYMfor symbol literals andOP_INTERNfor dynamic symbols, so this path mainly matters for.mrbfiles produced by other mruby toolchains that do emitOP_SYMBOL.Tests
InternFnv32CollidingNames: two documented FNV-1a 32-bit collision pairs intern to distinct symbols and stay findable in both directions.PoolSymbolMemoAcrossStates: a hand-assembledOP_SYMBOLirep executed on two states resolves the correct name in each (fails if one state reuses the other's memo).🤖 Generated with Claude Code