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

remove vec allocation in CRDS deserialize #5143

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+17 −3
Open

remove vec allocation in CRDS deserialize #5143

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95fc74a
remove vec allocation in CRDS deserialize
Mar 4, 2025
d8d544e
comments from review
Mar 5, 2025
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
20 changes: 17 additions & 3 deletions gossip/src/crds_value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,16 @@ use {
serde::de::{Deserialize, Deserializer},
solana_hash::Hash,
solana_keypair::{signable::Signable, Keypair},
solana_packet::PACKET_DATA_SIZE,
solana_pubkey::Pubkey,
solana_sanitize::{Sanitize, SanitizeError},
solana_signature::Signature,
solana_signer::Signer,
std::borrow::{Borrow, Cow},
std::{
borrow::{Borrow, Cow},
io::Cursor,
mem::MaybeUninit,
},
};

/// CrdsValue that is replicated across the cluster
Expand Down Expand Up @@ -227,8 +232,17 @@ impl<'de> Deserialize<'de> for CrdsValue {
data: CrdsData,
}
let CrdsValue { signature, data } = CrdsValue::deserialize(deserializer)?;
let bincode_serialized_data = bincode::serialize(&data).unwrap();
let hash = solana_sha256_hasher::hashv(&[signature.as_ref(), &bincode_serialized_data]);
// To compute the hash of the received CrdsData we need to re-serialize it
// PACKET_DATA_SIZE is always enough since we have just received the value in a packet
let mut buffer: MaybeUninit<[u8; PACKET_DATA_SIZE]> = MaybeUninit::uninit();
// SAFETY: we are only using this buffer to store serialize results
let buf_ref = unsafe { buffer.assume_init_mut() };
Comment on lines +237 to +239
Copy link

Choose a reason for hiding this comment

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

I don't think we should do this as we haven't actually initialized the buffer. We currently do something like this elsewhere in the codebase, but we shouldn't do it there either and PR's that have addressed that behavior have gotten hung up in review.

I think there are two approaches we can consider here:

  • Just do [0u8; PACKET_DATA_SIZE] like you previously had OR
  • Do something like I did in Introduce API to safely initialize Packets #3533 (which never merged 😆 )
    • This linked PR also has some conversation about why we shouldn't do assume_init() in one of the other places that we do

Not having to zero the buffer should seemingly have some gains. But, I think removing the alloc is the more significant gain so maybe it makes sense to keep this PR simpler for now

CC @alessandrod

Copy link
Author

Choose a reason for hiding this comment

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

@steviez I do not see a reason to zero-out this buffer. We are never actually reading any values from it for anything other than hashing. I agree the actual overhead is probably very small, but we are doing this up to 20K times per second. @alessandrod was the one who suggested that initializing it is not useful.

Copy link

Choose a reason for hiding this comment

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

I don't want to speak for Alessandro, but here is a comment from him on a very similar item on that same PR I linked: #3533 (comment)

If I understand correctly, I think his suggestion is something like:

  • Create MaybeUninit<[u8; PACKET_DATA_SIZE]> like you have it now
  • Serialize data into the buffer
    • Given that a MaybeUninit is written to with pointers, you can't directly call bincode::serialize_into() (unless I missed something) which is why I wrote that thin wrapper in my code
  • Fill the remainder of the buffer with 0's
  • Finally, do .assume_init() to mark that we have now fully initialized the buffer

let position = {
let mut cursor = Cursor::new(buf_ref.as_mut());
bincode::serialize_into(&mut cursor, &data).map_err(serde::de::Error::custom)?;
cursor.position() as usize
};
let hash = solana_sha256_hasher::hashv(&[signature.as_ref(), &buf_ref[0..position]]);
Ok(Self {
signature,
data,
Expand Down
Loading