perf(table): skip redundant first bytes.Equal in MergeIterator.Next#2281
Open
shaunpatterson wants to merge 2 commits into
Open
perf(table): skip redundant first bytes.Equal in MergeIterator.Next#2281shaunpatterson wants to merge 2 commits into
shaunpatterson wants to merge 2 commits into
Conversation
Invariant: every public method that leaves MergeIterator Valid ends with setCurrent(), which sets curKey = small.key. So on entry to Next(), curKey == small.key byte-for-byte, and the first iteration of the original duplicate-skip loop's bytes.Equal check is always true — we always need to advance once before checking for duplicates. Hoist the first small.next() + fix() out of the loop and keep checking for duplicates only on subsequent iterations. Saves one bytes.Equal call per emitted key on the merged-read hot path. Existing TestMergeDuplicates (forward + reverse) and TestMergeIterator* cover this code path. Benchmark impact (M4 Max, median of 3 runs): - BenchmarkReadMerged: -2.4% (~523M -> ~510M ns)
Calling Next() after a Seek past the end (iterator already invalid) must be a safe no-op. The early-return guard at the top of Next() previously had no coverage. Add a focused regression that Seeks past the end then calls Next(), verifying no panic and Valid()=false. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.
Summary
MergeIterator.Nextis called aftersetCurrent()has already copiedmi.small.keyintomi.curKey, so the very firstbytes.Equal(mi.small.key, mi.curKey)comparison inside the loop is always true. The same is true after everymi.small.next() + fix()advance that we then re-loop on: at that point we know the previous-small key equals curKey by virtue of how we entered the loop, but we haven't re-checked the new small.key.Restructure
Nextto make the first advance unconditional and only testbytes.Equalon subsequent passes. This drops onebytes.Equalcall (typically a SIMD-vectorized but still non-trivial library call) from everyNext()invocation — andNextis called for every key in a forward scan.The loop invariants are preserved: after the unconditional first advance, we loop while the new small.key still equals the saved curKey (i.e. another duplicate of the same user-key remains on either child).
Measurement
BenchmarkReadMerged: -5.0%ns/op, median of 3 runsTest plan
go test -short -race ./table/—TestMergeIteratorDuplicate,TestMergeDuplicates(forward + reverse),TestMergeMore,TestMergeIteratorSeek*all passgo vet ./...MergeIterator.Nextis at 100% statement coverage under the existing test suite (verified with-coverprofile).🤖 Generated with Claude Code