Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

behzadnouri
Copy link

@behzadnouri behzadnouri commented Feb 5, 2025

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.

@behzadnouri behzadnouri force-pushed the share-shred-payload-sigverify branch from c8f42a9 to a91c1d3 Compare February 5, 2025 17:01
@steviez
Copy link

steviez commented Feb 5, 2025

@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
Copy link
Author

behzadnouri commented Feb 5, 2025

@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
Copy link

steviez commented Feb 5, 2025

  • 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

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
Copy link
Author

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.

Comment on lines -105 to -116
let mut addrs: Vec<_> = self.addrs.iter().collect();
let reverse_count = |(_addr, count): &_| Reverse(*count);
if addrs.len() > MAX_NUM_ADDRS {
addrs.select_nth_unstable_by_key(MAX_NUM_ADDRS, reverse_count);
addrs.truncate(MAX_NUM_ADDRS);
}
addrs.sort_unstable_by_key(reverse_count);
info!(
"num addresses: {}, top packets by source: {:?}",
self.addrs.len(),
addrs
);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@behzadnouri behzadnouri force-pushed the share-shred-payload-sigverify branch from a91c1d3 to 3653ce0 Compare February 5, 2025 23:50
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.
@behzadnouri behzadnouri force-pushed the share-shred-payload-sigverify branch from 3653ce0 to 631996b Compare February 5, 2025 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants