perf: remove hashMap for optimized peer selection #80
Conversation
abhishek-singla-97
changed the title
Replace separate keyHashes []uint32 and hashMap map[uint32]string with a single segments []segment struct slice. This eliminates the map lookup overhead in the hot path (Get, findSegmentOwner). Benchmark results show 7-22% performance improvement: | Benchmark | Old (ns/op) | New (ns/op) | Speedup | |-------------------------------|-------------|-------------|--------------| | Get/segs50-shards8 | 37.99 | 34.47 | 9.3% faster | | Get/segs50-shards32 | 40.66 | 37.41 | 8.0% faster | | Get/segs50-shards512 | 71.94 | 65.40 | 9.1% faster | | GetReplicated/shards8-reps2 | 174.2 | 161.8 | 7.1% faster | | GetReplicated/shards512-reps2 | 242.9 | 203.8 | 16.1% faster | | GetReplicated/shards8-reps8 | 1061.0 | 823.9 | 22.4% faster | Geometric mean: 11.5% faster Memory allocations: unchanged
abhishek-singla-97
force-pushed
the
optimize-consistenthash-eliminate-hashmap
branch
from
043796f to
1488f8d
Compare
|
@dfinkel can you please take a look at this PR? I have mentioned the change in the description, which is a perf enhancement |
|
Thanks for sending this PR! Sorry about the delay. (We were on vacation for the end of the year) This does look like a nice improvement, and the code looks good at a first glance, I'll make a closer pass once the tests have passed. By the way, it's helpful to link to or reference the PR and/or commit from upstream when porting patches or reimplementing them. |
|
sure @dfinkel |
dfinkel
left a comment
dfinkel
left a comment
There was a problem hiding this comment.
Looks good!
Just a couple minor nits.
consistenthash/map_rangefunc.go
Outdated
| if a.hash < b.hash { | ||
| return -1 | ||
| } | ||
| if a.hash > b.hash { | ||
| return 1 | ||
| } | ||
| return 0 |
There was a problem hiding this comment.
this can be simplified by using cmp.Compare
slices.SortFunc(m.segments, func(a, b segment) int { return cmp.Compare(a.hash,b.hash)}| hashToOwner := make(map[uint32]string, len(m.segments)+len(ownerKeys)*m.segsPerKey) | ||
| for _, seg := range m.segments { | ||
| hashToOwner[seg.hash] = seg.owner | ||
| } |
There was a problem hiding this comment.
Let's add a comment saying that Add() is called several orders of magnitude less often than Get and GetReplicated, since hashrings should change slowly, so it's beneficial to trade a smaller Map against doing a bit more work when calling Add.
There was a problem hiding this comment.
added the comment. Kept the wording quite similar to what you mentioned. Hope that's fine.
| hashToOwner := make(map[uint32]string, len(m.segments)+len(ownerKeys)*m.segsPerKey) | ||
| for _, seg := range m.segments { | ||
| hashToOwner[seg.hash] = seg.owner | ||
| } |
|
I've tagged v1.4.1 with this change: |
2 participants
Summary
This PR optimizes the consistent hash implementation by replacing the separate
keyHashes []uint32slice andhashMap map[uint32]stringwith a singlesegments []segmentstruct slice. This eliminates the map lookup overhead in the hot path (Get(),findSegmentOwner()).Changes
segmentstruct combininghash uint32andowner stringkeyHashesandhashMapwithsegments []segmentfindSegmentOwner()to access segments directly (the key optimization)Add()to use a temporary map during insertion for collision handlingBenchmark Comparison
Memory allocations remain unchanged.
Motivation
Inspired by a similar optimization in groupcache (groupcache/groupcache-go#22). The key insight is that storing hash and owner together in a struct slice provides: