Optimize tree traversal performance - #213
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR refactors MaxMind DB search-tree traversal to extract IP bits using 32-bit big-endian chunks with up-front bounds checks, centralizes decoder pointer-chain resolution into a ChangesCore reader & decoder changes
Sequence Diagram(s)No sequence diagrams generated: the changes are primarily implementation refactoring of internal behavior, with mixed patterns affecting traversal loops, pointer resolution, caching, and reflection dispatch. Each checkpoint serves a distinct functional purpose without a singular primary flow to visualize. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/decoder/decoder.go`:
- Around line 388-409: In resolveCtrlData: avoid following pointer-to-pointer
chains — when you see KindPointer from d.d.decodeCtrlData, call
d.d.decodePointer once to get the target (and set nextOffset if not set), then
invoke d.d.decodeCtrlData exactly one more time on the pointer target; if that
second decode returns KindPointer, return an error (reject pointer-to-pointer)
instead of looping, otherwise return the resolved kind/size/dataOffset and
nextOffset. Ensure you reference resolveCtrlData, d.d.decodeCtrlData,
KindPointer, d.d.decodePointer and nextOffset when making the change.
In `@internal/decoder/string_cache.go`:
- Around line 60-66: The current check-and-store on sc.recentMisses[i] using
Load followed by Store is non-atomic and can violate the "second consecutive
miss" rule under concurrency; change the logic in the admission path around
sc.recentMisses[i], admissionValue, and entry.Store(&cacheEntry{...}) to perform
a single read-modify-write (use atomic.Swap or an atomic CAS loop) so you
atomically detect the prior value and only admit the string when the prior value
equals admissionValue; ensure the chosen atomic operation returns the previous
value so you can decide whether to call entry.Store or to instead update the
miss counter atomically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 18e25e05-a1dd-44a3-825e-2b841059aa4b
📒 Files selected for processing (14)
bad_data_test.gointernal/decoder/data_decoder.gointernal/decoder/decoder.gointernal/decoder/example_kind_test.gointernal/decoder/kind_test.gointernal/decoder/reflection.gointernal/decoder/string_cache.gointernal/decoder/type_assert_go125.gointernal/decoder/type_assert_pre125.gommap_windows.goreader.goreader_bounds_test.goreader_test.gotraverse.go
💤 Files with no reviewable changes (4)
- internal/decoder/type_assert_pre125.go
- internal/decoder/example_kind_test.go
- reader_bounds_test.go
- internal/decoder/type_assert_go125.go
Comment on lines
+60
to
+66
| if sc.recentMisses[i].Load() == admissionValue { | ||
| entry.Store(&cacheEntry{ | ||
| str: str, | ||
| offset: offset, | ||
| }) | ||
| } else { | ||
| sc.recentMisses[i].Store(admissionValue) |
There was a problem hiding this comment.
Keep the admission update atomic.
Splitting the check and write across Load/Store breaks the documented "second consecutive miss" rule under concurrency. An intervening miss can slip in between those operations, so cold strings can be admitted spuriously and the slot churn goes up. Use a single RMW (Swap or a CAS loop) here.
Proposed fix
admissionValue := uint64(offset) + 1
- if sc.recentMisses[i].Load() == admissionValue {
+ if sc.recentMisses[i].Swap(admissionValue) == admissionValue {
entry.Store(&cacheEntry{
str: str,
offset: offset,
})
- } else {
- sc.recentMisses[i].Store(admissionValue)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if sc.recentMisses[i].Load() == admissionValue { | |
| entry.Store(&cacheEntry{ | |
| str: str, | |
| offset: offset, | |
| }) | |
| } else { | |
| sc.recentMisses[i].Store(admissionValue) | |
| admissionValue := uint64(offset) + 1 | |
| if sc.recentMisses[i].Swap(admissionValue) == admissionValue { | |
| entry.Store(&cacheEntry{ | |
| str: str, | |
| offset: offset, | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/decoder/string_cache.go` around lines 60 - 66, The current
check-and-store on sc.recentMisses[i] using Load followed by Store is non-atomic
and can violate the "second consecutive miss" rule under concurrency; change the
logic in the admission path around sc.recentMisses[i], admissionValue, and
entry.Store(&cacheEntry{...}) to perform a single read-modify-write (use
atomic.Swap or an atomic CAS loop) so you atomically detect the prior value and
only admit the string when the prior value equals admissionValue; ensure the
chosen atomic operation returns the previous value so you can decide whether to
call entry.Store or to instead update the miss counter atomically.
Remove hasBufferRange calls from the inner loops of traverseTree24/28/32. A single safety check is added at the start of each function to ensure the buffer is large enough for the entire search tree. This maintains safety while reducing overhead during IP lookups.
Process the 16-byte IPv6 address in 32-bit chunks using BigEndian.Uint32. This reduces bit-shifting arithmetic and improves data locality during tree traversal.
Replace manual bit-shifting of individual slice elements with binary.BigEndian.Uint32. This simplifies the code in traverseTree24, traverseTree28, and traverseTree32 while maintaining equivalent performance.
Use netip.AddrFrom4 with slice-to-array conversion to avoid the overhead of slice validation in netip.AddrFromSlice.
Disable gomodguard in .golangci.yml since it is deprecated and generates a warning on stderr, which blocks the git pre-commit hook.
Replace the byte-by-byte loop with a direct slice-to-array comparison against a zero-initialized array. This makes the code cleaner and more declarative.
Deduplicate tree traversal by reusing the traverseTree function to find the IPv4 start node in setIPv4Start. This removes the duplicate loop and logic.
Remove redundant type_assert_go125.go and type_assert_pre125.go, defining tryTypeAssert directly in reflection.go using Go 1.25's reflect.TypeAssert feature.
Consolidate makeAddressable and newAddressableValue into a single makeAddressable function to eliminate duplicate code.
Merge reader_bounds_test.go into bad_data_test.go, and merge internal/decoder/example_kind_test.go into internal/decoder/kind_test.go. This reduces the number of redundant test files and keeps the test suite focused and clean.
Construct the 16-byte representation using a sparse array literal instead of a temporary array and copy helper. This avoids the overhead of copy and slice bounds checks, making v4ToV16 over 4x faster. Also add a round-trip test (TestV4ToV16RoundTrip) covering both v4ToV16 and v6ToV4 directly, replacing the prior indirect coverage through lookup parity tests.
Use a shared helper to resolve decoder control data through pointer chains. This keeps the pointer-following logic in one place for PeekKind, Offset, and decodeCtrlDataAndFollow, and documents how the helper differs from decodeCtrlData.
Collapse the corrupt-pointer checks into a single resolved-offset bounds check while preserving the existing behavior on invalid pointers.
Reuse the checked dataSectionStart/dataSectionEnd bounds when creating the decoder instead of recomputing the slice indices.
Keep OpenBytes option processing on a local readerOptions value instead of allocating a pointer up front.
Keep NetworksWithin option processing on a local networkOptions value inside the iterator instead of taking a pointer literal up front.
Check the successful node > nodeCount case first and handle the empty-record sentinel separately without changing the invalid-node fallback.
Replace the atomic Swap call in internAt with a conditional Load and Store check. This avoids read-modify-write LOCK instructions on cache misses, reducing bus locks. Benchmark impact: Single-threaded stringCache benchmarks are slightly faster (Hot path: ~1.08 ns/op vs ~1.12 ns/op, ~3% speedup). Concurrent lookups are clean under -race and benefit from reduced LOCK instruction overhead on misses.
Avoid slice allocations on the happy path in decodeValueImpl by tracking up to two allocated pointer values in scalar variables. Falls back to a slice only when pointer nesting depth exceeds two. Benchmark impact: Avoids heap slice allocations entirely during successful decoding into pointer fields. While CityLookup (which decodes into a pointer-less benchmarkCity struct) is unaffected, this library-wide optimization prevents GC and allocation overhead for pointer-heavy custom destination structs.
Fast-path key decoding when the control byte represents a pointer that resolves directly to a string under 29 bytes. This avoids calling decodeCtrlData and decodePointer twice for heavily duplicated keys. Benchmark impact: Yields a substantial **8.0% speedup** on BenchmarkCityLookup (reducing it from 1485 ns/op to 1366 ns/op) and a **5% to 7% throughput increase** on concurrent lookups, as it avoids calling decodeCtrlData and decodePointer twice for heavily duplicated pointer keys.
Add BenchmarkCityLookupWithPointers to measure allocations and speed when decoding into pointer-heavy structures, and BenchmarkCityLookupOnlyIPv6 to measure traversal performance for IPv6.
Document the pointer-decoding allocation savings, key-decoder pointer fast path, IPv6 tree traversal speedups, and stack-allocation changes.
This was referenced Jul 12, 2026
6 tasks
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
1 participant
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.
This PR introduces two performance optimizations for tree traversal in the MMDB reader:
hasBufferRangecalls in the inner loops. This improves lookup speed by ~5-8%.binary.BigEndian.Uint32reduces bit-shifting overhead and improves data locality.Benchmarks show consistent improvements across standard lookups, with up to ~10% gain on specific decoding paths.
Summary by CodeRabbit
Performance
Bug Fixes