More efficient sort in tryRelocateShard
#128063
Conversation
No need to do this via an allocation-heavy `Stream`, we can just put the objects straight into an array, sort them in-place, and keep hold of the array to avoid having to allocate anything on the next iteration. Also slims down `BY_DESCENDING_SHARD_ID`: it's always sorting the same index so we don't need to look at `ShardId#index` in the comparison, nor do we really need multiple layers of vtable lookups, we can just compare the shard IDs directly. Relates elastic#128021
DaveCTurner
added
>enhancement
:Distributed Coordination/Allocation
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Hi @DaveCTurner, I've created a changelog YAML for you. |
DaveCTurner
added
the
auto-backport
There was a problem hiding this comment.
lgtm 👍
To confirm, this change essentially
- removes the use of Stream, which isn't performant
- shortens the comparison method
- reuses allocated memory
Though this last one sacrifices memory in favor of performance -- if a user has one index with a stupid number of shards, we're keep that array memory allocation for the duration of a balancing round? Though even a large number of shards will take insignificant memory.
| for (final var shardRouting : index) { | ||
| if (shardRouting.started()) { // cannot rebalance unassigned, initializing or relocating shards anyway | ||
| shardRoutingsOnMaxWeightNode[startedShards] = shardRouting; | ||
| startedShards += 1; |
There was a problem hiding this comment.
opt: ++startedShards or shardRoutingsOnMaxWeightNode[startedShards++] = shardRouting;
There was a problem hiding this comment.
IMO unary increments are harder on the reader than spelling out a += 1 so I'd rather leave it like this.
DaveCTurner
added
the
auto-merge-without-approval
Yeah the maximum is 1024, if they all end up on one node for some reason then we allocate an array of length 2048 which is still tiny and probably not worth GC'ing. We probably allocate more than 2048*8=16kiB today just by using streams. |
No need to do this via an allocation-heavy `Stream`, we can just put the objects straight into an array, sort them in-place, and keep hold of the array to avoid having to allocate anything on the next iteration. Also slims down `BY_DESCENDING_SHARD_ID`: it's always sorting the same index so we don't need to look at `ShardId#index` in the comparison, nor do we really need multiple layers of vtable lookups, we can just compare the shard IDs directly. Relates elastic#128021
💚 Backport successful
|
No need to do this via an allocation-heavy `Stream`, we can just put the objects straight into an array, sort them in-place, and keep hold of the array to avoid having to allocate anything on the next iteration. Also slims down `BY_DESCENDING_SHARD_ID`: it's always sorting the same index so we don't need to look at `ShardId#index` in the comparison, nor do we really need multiple layers of vtable lookups, we can just compare the shard IDs directly. Relates #128021
No need to do this via an allocation-heavy
Stream, we can just put theobjects straight into an array, sort them in-place, and keep hold of the
array to avoid having to allocate anything on the next iteration.
Also slims down
BY_DESCENDING_SHARD_ID: it's always sorting the sameindex so we don't need to look at
ShardId#indexin the comparison, nordo we really need multiple layers of vtable lookups, we can just compare
the shard IDs directly.
Relates #128021