Balancer skips disk related computation when disk weight factor is zero #136352
Conversation
Flamegraph shows Balancer instantiation takes consider amount of time in an allocate call. More than 1/4 of the instantiation time is for computing disk related stats which is wasteful when the disk weight factor is zero. This PR skips these computations in such case.
ywangd
added
>non-issue
:Distributed Coordination/Allocation
| avgDiskUsageInBytesPerNode = skipDiskUsageCalculation | ||
| ? 0 | ||
| : WeightFunction.avgDiskUsageInBytesPerNode(allocation.clusterInfo(), metadata, routingNodes); | ||
| nodes = Collections.unmodifiableMap(buildModelFromAssigned(skipDiskUsageCalculation)); |
There was a problem hiding this comment.
The cost saving is realistic. My main question is whether the approach is considered hacky.
There was a problem hiding this comment.
What about if rather than having the additional flag, we passed the weighting around, and the expensive parts could only perform the calculation if the weighting was non-zero?
I'm not sure if that's better, but it's a thought.
There was a problem hiding this comment.
We could refer to this.balancingWeights perhaps?
There was a problem hiding this comment.
See below flamegraph (from many-shards benchmark) that shows the time spent on disk related computation (purple color) inside allocate calls.
There was a problem hiding this comment.
We synced offline and agreed to change the boolean flag to be a method on BalancingWeights.
…or is zero" This reverts commit e6eef9e.
elasticsearchmachine
added
the
serverless-linked
…tation-conditionally
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
| Map.of(), | ||
| Map.of(), | ||
| Map.of() | ||
| ); |
There was a problem hiding this comment.
Nit: could use ClusterInfo.builder().shardSizes(...).build()?
There was a problem hiding this comment.
I forgot there is builder. Pushed b7e22fc looks much nicer! Thanks!
| private float maxShardSizeBytes(ProjectIndex index) { | ||
| if (balancingWeights.diskUsageIgnored()) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
This bit I have minor apprehensions about. But it seems like it's not an easy one to skip on the caller side. And we do have that information available to us here via the balancingWeights.
There was a problem hiding this comment.
I can remove this change if you prefer. It does not really show up in the flamegraph. So I could be over-zealous.
There was a problem hiding this comment.
Yeah maybe that would be nicer. The behaviour seems a little surprising potentially. It would seem safer if the caller was skipping the call rather than the callee just returning zero.
There was a problem hiding this comment.
It turns out that we can check it at the call site. Not sure why I initially thought it was not feasible ... Pushed 0affb99
Let me know if this works for you. Thanks!
3 participants
Flamegraph shows Balancer instantiation takes consider amount of time in an allocate call. More than 1/4 of the instantiation time is for computing disk related stats which is wasteful when the disk weight factor is zero. This PR skips these computations in such case.