added flat trie #7
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new FlatTrie data structure implementation alongside the existing SimpleTrie, and renames the generic trie constructors to be more specific. The FlatTrie uses arena-based allocation with contiguous memory chunks (sizes 4, 16, 64, and 256) to improve cache locality and performance compared to the pointer-based SimpleTrie.
Key changes:
- Added
FlatTrieimplementation with arena-based memory allocation and node promotion strategy - Renamed
NewTrietoNewSimpleTrieandNewTrieFromMaptoNewSimpleTrieFromMapfor clarity - Removed support for
uint,int,~uint, and~inttypes from trie key constraints - Simplified string converter type assertion logic
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| simple_trie.go | Renamed constructor functions to include "Simple" prefix for specificity |
| simple_trie_test.go | Updated all test function calls to use renamed SimpleTrie constructors |
| flat_trie.go | Added new FlatTrie implementation with arena-based memory management |
| flat_trie_test.go | Added comprehensive test suite for FlatTrie mirroring SimpleTrie tests |
| flat_node.go | Added flatNode struct and arena size calculation helper |
| converter.go | Simplified string converter Load method by removing redundant type switch |
| contracts.go | Removed uint and int types from TrieInteger and TrieSlice interfaces |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
flat_trie.go
Outdated
| fmt.Println("expanding arena 4") | ||
| this.arena4, this.useTable4 = expandArena(this.arena4, this.useTable4) | ||
| case arenaNumber16: | ||
| fmt.Println("expanding arena 16") | ||
| this.arena16, this.useTable16 = expandArena(this.arena16, this.useTable16) | ||
| case arenaNumber64: | ||
| fmt.Println("expanding arena 64") | ||
| this.arena64, this.useTable64 = expandArena(this.arena64, this.useTable64) | ||
| case arenaNumber256: | ||
| fmt.Println("expanding arena 256") |
There was a problem hiding this comment.
Debug print statements should be removed from production code. These fmt.Println calls in the doubleArenaSize method (lines 309, 312, 315, 318) will clutter output and should either be removed or replaced with proper logging.
| fmt.Println("expanding arena 4") | |
| this.arena4, this.useTable4 = expandArena(this.arena4, this.useTable4) | |
| case arenaNumber16: | |
| fmt.Println("expanding arena 16") | |
| this.arena16, this.useTable16 = expandArena(this.arena16, this.useTable16) | |
| case arenaNumber64: | |
| fmt.Println("expanding arena 64") | |
| this.arena64, this.useTable64 = expandArena(this.arena64, this.useTable64) | |
| case arenaNumber256: | |
| fmt.Println("expanding arena 256") | |
| this.arena4, this.useTable4 = expandArena(this.arena4, this.useTable4) | |
| case arenaNumber16: | |
| this.arena16, this.useTable16 = expandArena(this.arena16, this.useTable16) | |
| case arenaNumber64: | |
| this.arena64, this.useTable64 = expandArena(this.arena64, this.useTable64) | |
| case arenaNumber256: |
simple_trie_test.go
Outdated
| benchy.New(b, options.Medium). | ||
| RegisterBenchmark("map", provider.WrapBenchmarkFunc(func(a, b string) { | ||
| _ = lookupMap[strings.ToLower(string(a))] == statesMap[strings.ToLower(string(b))] | ||
| _ = lookupMap[a] == statesMap[b] |
There was a problem hiding this comment.
The benchmark comparison logic has been simplified by removing strings.ToLower() calls, but this changes the benchmark semantics. The lookupMap was built with lowercase keys (line 395), so comparing lookupMap[a] without lowercasing a will produce incorrect results compared to the original benchmark that used strings.ToLower(string(a)).
| _ = lookupMap[a] == statesMap[b] | |
| _ = lookupMap[strings.ToLower(a)] == statesMap[b] |
No description provided.