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

packet: Use Bytes instead of an array in Packet #25

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

Conversation

vadorovsky
Copy link
Member

@vadorovsky vadorovsky commented Feb 11, 2025

Agave PR using this one anza-xyz/agave#4966


The reason behind the switch is that Packet gets cloned in multiple places. Bytes provides a zero-copy abstraction, where calling clone() doesn't make an actual copy of the underlying data and all instances point to the same memory.

However, the old layout containing a sized array is convenient for CUDA. To not break CUDA support, this change introduces a new struct called PacketArray, which a Packet can be converted into just before calling GPU-based sigverify. That requires a copy, ideally just one.

At the same time, CPU-based sigverify and all the other components are going to benefit from zero-copy properties of Bytes.

@vadorovsky vadorovsky force-pushed the packet-bytes branch 11 times, most recently from 6129f4e to ed55378 Compare February 13, 2025 03:38
@vadorovsky vadorovsky changed the title packet: Use Bytes instead of an array packet: Use Bytes instead of an array in Packet Feb 13, 2025
@vadorovsky vadorovsky force-pushed the packet-bytes branch 8 times, most recently from 6476266 to a3c9c09 Compare February 13, 2025 12:27
@vadorovsky vadorovsky force-pushed the packet-bytes branch 2 times, most recently from b2512ba to 64d0a6c Compare February 14, 2025 03:43
@behzadnouri
Copy link
Contributor

Can you please provide the evidence that this is a good change?

We can theoretically argue that one thing is better than the other but when testing it a lot of times the observation do not match the expectation and intuition. Besides that there are so many downsides with using Bytes as already enumerated here:
anza-xyz/agave#3817 (comment)

  • Bytes does dynamic dispatch which is pretty slow, particularly so in certain runtime access patterns.
  • We already use Recycler for packets which Bytes is not compatible with (unless we do memcopies anyways). So not even sure we will do fewer allocations or memcopies with Bytes.
  • Bytes is pushing out the gpu code apparently. Again, sigverify is a bigger bottleneck than an allocation or memcopy.
  • Bytes does not work with [u8; N], so you are always forced an extra redirection (Packet is just a simple [u8; N] wrapper). Bytes also does not work with Arc<Vec<u8>> either.

@steviez
Copy link
Contributor

steviez commented Feb 17, 2025

More of an administrative note, I think there has been discussion around Packet not belonging in the SDK. If that sentiment remains, we might reintroduce the type in Agave + mark the type deprecated here. I'll try to help figure out the current sentiment on that; I'd say you can proceed with anything as-is and we can discuss shifting it to another repo as a last step

@vadorovsky vadorovsky force-pushed the packet-bytes branch 3 times, most recently from b66cb8b to 3eaa076 Compare February 20, 2025 13:21
The reason behind the switch is that `Packet` gets cloned in multiple
places. `Bytes` provides a zero-copy abstraction, where calling
`clone()` doesn't make an actual copy of the underlying data and all
instances point to the same memory.

However, the old layout containing a sized array is convenient for CUDA.
To not break CUDA support, this change introduces a new struct called
`PacketArray`, which a `Packet` can be converted into just before
calling GPU-based sigverify. That requires a copy, ideally just one.

At the same time, CPU-based sigverify and all the other components are
going to benefit from zero-copy properties of `Bytes`.
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.

3 participants