Copilot commented on code in PR #3415:
URL: https://github.com/apache/dubbo-go/pull/3415#discussion_r3409064029
##########
cluster/loadbalance/leastactive/loadbalance.go:
##########
@@ -57,14 +60,25 @@ func (lb *leastActiveLoadBalance) Select(invokers
[]base.Invoker, invocation bas
}
var (
- leastActive int32 = -1 // The least active
value of all invokers
- totalWeight int64 // The number of
invokers having the same least active value (LEAST_ACTIVE)
- firstWeight int64 // Initial value, used
for comparison
- leastCount int // The number of
invokers having the same least active value (LEAST_ACTIVE)
- leastIndexes = make([]int, count) // The index of
invokers having the same least active value (LEAST_ACTIVE)
- sameWeight = true // Every invoker has
the same weight value?
- weights = make([]int64, count) // The weight of every
invokers
+ leastActive int32 = -1 // The least active value of all
invokers
+ totalWeight int64 // The number of invokers having the
same least active value (LEAST_ACTIVE)
+ firstWeight int64 // Initial value, used for comparison
+ leastCount int // The number of invokers having the
same least active value (LEAST_ACTIVE)
Review Comment:
In least-active selection, `totalWeight` is used as the sum of weights for
invokers with the least active value (it’s incremented by `afterWarmup`), but
the inline comment describes it as a count. Updating the comment will prevent
future confusion/misuse when refactoring this code path.
##########
cluster/loadbalance/loadbalance_benchmarks_test.go:
##########
@@ -84,3 +97,38 @@ func BenchmarkRandomLoadbalance(b *testing.B) {
func BenchmarkAliasMethodLoadbalance(b *testing.B) {
Benchloadbalance(b,
extension.GetLoadbalance(constant.LoadBalanceKeyAliasMethod))
}
+
+func benchmarkLoadBalanceSmallMedium(b *testing.B, lb loadbalance.LoadBalance)
{
+ b.Helper()
+ for _, count := range []int{2, 4, 8, 16, 32, 33} {
+ for _, weighted := range []bool{false, true} {
+ name := fmt.Sprintf("invokers=%d", count)
+ if weighted {
+ name += "/weighted"
+ } else {
+ name += "/uniform"
+ }
+ b.Run(name, func(b *testing.B) {
+ invokers := generateInvokers(count, weighted)
+ invocation := &invocation.RPCInvocation{}
+ b.ReportAllocs()
+ b.ResetTimer()
+ for i := 0; i < b.N; i++ {
+ lb.Select(invokers, invocation)
+ }
+ })
Review Comment:
In the small/medium benchmark, the local variable name `invocation` shadows
the imported `invocation` package name. This makes the code harder to read and
prevents using the package identifier later in the closure without renaming.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
