perf(client): prevent unneeded memory copy in BlobWitnessProvider #5
Conversation
| @@ -44,7 +44,7 @@ impl<T: BlobProvider + Send> BlobProvider for BlobWitnessProvider<T> { | |||
| ) -> Result<Vec<Box<Blob>>, Self::Error> { | |||
| let blobs = self.provider.get_blobs(block_ref, blob_hashes).await?; | |||
| for blob in &blobs { | |||
| let c_kzg_blob = c_kzg::Blob::from_bytes(blob.as_slice()).unwrap(); | |||
There was a problem hiding this comment.
This isn't the only host-side occurrence of using from_bytes.
| @@ -44,7 +44,7 @@ impl<T: BlobProvider + Send> BlobProvider for BlobWitnessProvider<T> { | |||
| ) -> Result<Vec<Box<Blob>>, Self::Error> { | |||
| let blobs = self.provider.get_blobs(block_ref, blob_hashes).await?; | |||
| for blob in &blobs { | |||
| let c_kzg_blob = c_kzg::Blob::from_bytes(blob.as_slice()).unwrap(); | |||
| let c_kzg_blob = c_kzg::Blob::new(***blob); | |||
There was a problem hiding this comment.
I believe this change makes the copy implicit (via Copy trait) racer than explicit via from slice.
There was a problem hiding this comment.
ah you might be right!
#[repr(C)]
#[derive(Debug, Hash, PartialEq, Eq)]
pub struct Blob {
bytes: [u8; 131072usize],
}
Could the fact that its C representation mean its actually moved and not copied? If not, then you're right and I will just close this PR.
There was a problem hiding this comment.
Feel like c-kzg should have a move constructor though...!
There was a problem hiding this comment.
The memory representation doesn't affect the borrow checker afaik
There was a problem hiding this comment.
After reading a bit more, pretty convinced also this is true. But also don't think the borrow checker and the fact that a struct implements the Copy trait actually means it will be copied when passed into a function that doesn't modify it, because those are lower level concerns. Rust passes arguments by value, but I think it is also able to perform https://en.wikipedia.org/wiki/Copy_elision like C++. Hence I do think the change in this PR could help (probably only when building with --release to have compiler optimizations). Nonetheless, this change would make a lot more sense if I implemented it uniformly across the codebase, and wrote some benchmark to actually show that it helps. So I'll close for now. Enjoy your holiday break! :)
from_bytescreates a copy of the passed slice. We can instead just usenewdirectly which takes ownership of the blob, and prevents the copy, since we don't need it afterwards.