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.

Sign up for GitHub

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

perf(client): prevent unneeded memory copy in BlobWitnessProvider #5

Closed
wants to merge 1 commit into from
Closed

perf(client): prevent unneeded memory copy in BlobWitnessProvider #5

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/client/src/witness.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't the only host-side occurrence of using from_bytes.

let c_kzg_blob = c_kzg::Blob::new(***blob);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this change makes the copy implicit (via Copy trait) racer than explicit via from slice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel like c-kzg should have a move constructor though...!

Copy link
Collaborator

Choose a reason for hiding this comment

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

The memory representation doesn't affect the borrow checker afaik

Copy link
Contributor Author

@samlaf samlaf Dec 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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! :)

let settings = alloy::consensus::EnvKzgSettings::default();
let commitment =
c_kzg::KzgCommitment::blob_to_kzg_commitment(&c_kzg_blob, settings.get())
Expand Down