shares shreds' payload between window-service and retransmit-stage

[behzadnouri]

Problem

Shreds received from turbine are concurrently sent to window-service to be deserialized and inserted into blockstore, while their payload is sent to retransmit-stage. Using a shared payload between the two concurrent paths will reduce allocations and memcopies.

Summary of Changes

The commit shares shreds' payload between window-service and retransmit-stage.

[steviez]

@vadorovsky has been looking into a possible change to back Packet with Bytes instead of the fixed sized array. I would have to let Michal comment more on the specifics, but such a change might allow us to accomplish the same thing as this PR without ever copying the payload out of the original Packet buffer.

[behzadnouri]

@vadorovsky has been looking into a possible change to back Packet with Bytes instead of the fixed sized array. I would have to let Michal comment more on the specifics, but such a change might allow us to accomplish the same thing as this PR without ever copying the payload out of the original Packet buffer.

I would not like to tie this with that other work (which I am not even confident is the right thing to do).
Replacing inner buffer in Packet with Bytes is a huge change, and it is not necessarily a good change because

• Bytes requires dynamic dispatch which is pretty slow.
• We already have a recycler for Packets. That will not work well with Bytes or will require to make a lot of clones which defeats the whole point of using Bytes anyways.
• The gpu code will break, which is definitely a negative trade-off (we spend a lot more time on sigverify than copying bytes). It is true that the gpu code is not used much today, but that can change in the future when the demand goes up.
• Bytes does not work with [u8; N] or Arc<Vec<u8>>. I am already using Arc<Vec<u8>> in the payload and I have plans to use [u8; N] in the payload as well: allows using fixed size arrays inside shred::Payload #4792

[steviez]

• We already have a recycler for Packets. That will not work well with Bytes or will require to make a lot of clones which defeats the whole point of using Bytes anyways.

The recyclers operate on PacketBatch right ? I think the idea would be one big Bytes allocation for PacketBatch, and each Packet within that batch gets a slice of that one. I think Bytes would handle the ref counting properly to ensure PacketBatch is dropped only after the individual Bytes references are all dropped

• The gpu code will break, which is definitely a negative trade-off (we spend a lot more time on sigverify than copying bytes). It is true that the gpu code is not used much today, but that can change in the future when the demand goes up.

As an FYI, I think we're leaning pretty heavily towards ripping the GPU code out: #3817

• Bytes does not work with [u8; N] or Arc<Vec<u8>>. I am already using Arc<Vec<u8>> in the payload and I have plans to use [u8; N] in the payload as well: allows using fixed size arrays inside shred::Payload #4792

Got it, we can discuss over there

I would not like to tie this with that other work (which I am not even confident is the right thing to do).

Fair enough. We can optimize things as they are and if Bytes ends up happening / being a good choice, we can refactor this if there are any gains to be had

[behzadnouri]

The recyclers operate on PacketBatch right ? I think the idea would be one big Bytes allocation for PacketBatch, and ...

Still does not sound like to me it will work with the recycler.

As an FYI, I think we're leaning pretty heavily towards ripping the GPU code out: #3817

That is terrible. Sigverify is more of a major bottleneck than whatever #3817 is going to solve.

We can optimize things as they are and if Bytes

Not using Bytes is good optimization. It requires dynamic dispatch which is pretty slow.

[behzadnouri]

Removing this info! log (and associated addrs bookkeeping) here because it is pretty inefficient to collect emit these logs here.
I will look into putting something similar elsewhere in the pipeline (maybe shred-fetch-stage or sigverify).

[AshwinSekar]

lgtm

[mergify]

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.