Recovery buffer size 16B smaller

[henningandersen]

G1GC will use humongous allocations when an allocation exceeds half the
chosen region size, which is minimum 1MB. By reducing the recovery
buffer size by 16 bytes we ensure that the recovery buffer is never
allocated as a humongous allocation.

[elasticmachine]

Pinging @elastic/es-distributed (:Distributed/Recovery)

[original-brownbear]

Does this really matter much for recoveries? We only have a very limited number of these arrays in existence at any given time anyway and heavily reuse them don't we? (not that I see any downside to the change otherwise :) just wondering)

[henningandersen]

We generate one such byte array per shard recovery on source. Not a lot of activity, but still I felt like the right thing to do here was to make the buffer 16 bytes smaller since it is unlikely to affect recovery speed.

A separate issue is that as far as I can see we allocate new byte arrays currently on the target for every chunk, since RecoveryFileChunkRequest calls in.readBytesReference(), which allocates a new byte array. This sounds like something we should fix separately, but this PR also lowers the impact of that for G1GC in the mean-time. I can double check this part tomorrow.

[Tim-Brooks]

A separate issue is that as far as I can see we allocate new byte arrays currently on the target for every chunk, since RecoveryFileChunkRequest calls in.readBytesReference(), which allocates a new byte array. This sounds like something we should fix separately, but this PR also lowers the impact of that for G1GC in the mean-time. I can double check this part tomorrow.

This is correct. I have been working on a mechanism to prevent these allocations (probably for peer recovery file chunks and index operations). It should be opened pretty soon.

[Tim-Brooks]

We generate one such byte array per shard recovery on source.

Also it looks like this is no longer true (I am confident it used to be true). It looks like when we made the recovery process async, we now do an allocate and copy in AsyncRecoveryTarget.

[Tim-Brooks]

Scratch my last comment, it looks like that async thing is only for testing simulation purposes.

[original-brownbear]

I'm not convinced this will have much if any practical impact but I don't see a downside either
=> LGTM :)