wrr: improve randomWRR performance #5067
Conversation
huangchong94
force-pushed
the
optimize-wrr-random
branch
2 times, most recently
from
61ee291
to
4dc83ca
Compare
huangchong94
changed the title
* use binary search when weights are not equal * random pick one when weights are equal
huangchong94
force-pushed
the
optimize-wrr-random
branch
from
4dc83ca
to
bdd88ac
Compare
huangchong94
changed the title
huangchong94
force-pushed
the
optimize-wrr-random
branch
from
d2dac86
to
ca75ed0
Compare
huangchong94
force-pushed
the
optimize-wrr-random
branch
from
ca75ed0
to
b52eec9
Compare
menghanl
added
the
Type: Performance
internal/wrr/random.go
Outdated
| Item interface{} | ||
| Weight int64 | ||
| Item interface{} | ||
| // TODO Delete Weight? This field is not necessary for randomWRR to work. |
There was a problem hiding this comment.
Delete this field if it's not used.
And it seems all fields in this struct can be unexported, right? Can you also change that? Thanks
internal/wrr/random.go
Outdated
| @@ -51,27 +57,35 @@ var grpcrandInt63n = grpcrand.Int63n | |||
| func (rw *randomWRR) Next() (item interface{}) { | |||
| rw.mu.RLock() | |||
| defer rw.mu.RUnlock() | |||
| if rw.sumOfWeights == 0 { | |||
| sumOfWeights := rw.items[len(rw.items)-1].AccumulatedWeight | |||
There was a problem hiding this comment.
This will fail if there's no item, right? (And add a test)
internal/wrr/wrr_test.go
Outdated
| func BenchmarkRandomWRRNext(b *testing.B) { | ||
| for _, n := range []int{100, 500, 1000} { | ||
| b.Run("equal-weights-"+strconv.Itoa(n)+"-items", func(b *testing.B) { | ||
| w := NewRandom().(*randomWRR) |
There was a problem hiding this comment.
Why do you need this type assertion?
…e unexported; add comment for field equalWeights in randomWRR; fix panic when there is no item in randomWRR.Next; add test case for wrr; remove redundant type assertion in BenchmarkRandomWRRNext;
| return rw.items[len(rw.items)-1].Item | ||
| sumOfWeights = rw.items[len(rw.items)-1].accumulatedWeight | ||
| // Random number in [0, sumOfWeights). | ||
| randomWeight := grpcrandInt63n(sumOfWeights) |
There was a problem hiding this comment.
Hmm, this may still fail if sumOfWeights == 0 (this happens when the items added all are weight 0 🤷♂️ )
Maybe keep the check if sumOfWeights == 0
There was a problem hiding this comment.
If items added all are weight 0, then rw.equalWeights will be true, Next will return normally at line 61 return rw.items[grpcrandInt63n(int64(len(rw.items)))].item
I considered keeping if sumOfWeights == 0 return nil, but the behaviour will be different from edf.go, in edf, if all
item's weights are zero, Next will return non nil item
internal/wrr/random.go
Outdated
| lastItem := rw.items[len(rw.items)-1] | ||
| accumulatedWeight = lastItem.accumulatedWeight + weight | ||
| lastItemWeight := lastItem.accumulatedWeight | ||
| if len(rw.items) > 1 { |
There was a problem hiding this comment.
Oh, I now see what you meant in the old comments. weight is used once here.
I would be OK to keep the weight field to simplify the code here.
Either way is fine.
There was a problem hiding this comment.
Added weight back to weightedItem to simplify code here
There was a problem hiding this comment.
By the way, why weight type is int64 not uint32, int64 will allow user add negative weight item to WRR which will result undefined behaviour in Next.
This PR improves
randomWRR.Nextperformance byIn general, this PR performs better than current implementation and has a more stable performance characteristics while the previous approach is more sensitive to weights distribution but it works better in some cases (i.e. small number of items with different weights)
RELEASE NOTES: wrr: improve randomWRR performance