Benchmark batch lookups vs. MapIterator, remove MapIterator.prevKey #1077
Conversation
This file contains 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
lmb
approved these changes
Jun 26, 2023
lmb
reviewed
Jun 26, 2023
| // non-nil interface and try to marshal it. | ||
| nextKey, mi.err = mi.target.NextKeyBytes(nil) | ||
|
|
||
| mi.curKey = make([]byte, mi.target.keySize) |
There was a problem hiding this comment.
Maybe a bit weird: for an empty map we get mi.curKey != nil due to this. Not sure if it's worth fixing though.
There was a problem hiding this comment.
Thanks for pointing it out, I think that's okay for now. No other methods depend on this state.
lmb
approved these changes
Jun 26, 2023
Benchmark results at time of writing: BenchmarkIterate/MapIterator-16 866 1457681 ns/op 64126 B/op 4004 allocs/op BenchmarkIterate/MapIteratorDelete-16 408 2526392 ns/op 80210 B/op 6004 allocs/op BenchmarkIterate/BatchLookup-16 59647 19974 ns/op 32851 B/op 8 allocs/op BenchmarkIterate/BatchLookupAndDelete-16 10000 154757 ns/op 32872 B/op 8 allocs/op BenchmarkIterate/BatchDelete-16 8306 136770 ns/op 16416 B/op 3 allocs/op Signed-off-by: Timo Beckers <timo@isovalent.com>
Change the terminology used in the iterator to revolve around the 'current' and the 'next' key instead of considering the 'previous' key. I found the behaviour after passing a nil `mi.prevKey` to NextKeyBytes a little subtle. If the interface value passed to NextKeyBytes contains a nil 'any', it will successfully detect nilness and not try to marshal the key. If the caller passes a []byte(nil), the nil check will fail and nextKey() will try to marshal the key. Perform a nil check on mi.curKey instead and explicitly pass an untyped nil to NextKeyBytes if the Map's first key needs to be looked up. This means prevKey can be eliminated. Signed-off-by: Timo Beckers <timo@isovalent.com>
ti-mo
force-pushed
the
tb/bench-map-iterate
branch
from
65b1353
to
452a683
Compare
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.
Since
MapIteratoris an interesting target for optimization through the upcoming zero-copy map ops work, add a benchmark and simplify its internal representation a bit.