removes repeated bincode serialization of gossip CrdsValues #3575

behzadnouri · 2024-11-11T15:19:27Z

Problem

Gossip CrdsValues are deserialized when received as a gossip push message or pull response, but serialized again immediately to obtain a value hash during Crds::insert and then again serialized repeatedly every time the value is pushed to another node or returned as a response to a pull request.

Summary of Changes

In order to avoid repeated serialization of a CrdsValue during its lifetime, the commit manually implements bincode (de)serialization of CrdsValue to hold on to bincode serialized bytes of CrdsData and reuses that for serializing CrdsValue.

behzadnouri · 2024-11-13T21:18:47Z

Left side is this code.

…_packet} As part of anza-xyz#3575 we need to implement bincode (de)serialization without deriving serde Serialize/Deserialize trait. Packet::{from_data,populate_packet} methods also only require that the data to be bincode serializable and do not care about serde either. Next version of bincode provides the necessary traits to achieve this functionality: https://docs.rs/bincode/2.0.0-rc.3/bincode/enc/trait.Encode.html However that is not released yet. In order to avoid serde::Serialize trait bound on Packet::{from_data,populate_packet}, the commit adds a new Serialize trait which only requires implementing bincode serialization.

…_packet} As part of anza-xyz#3575 we need to implement bincode (de)serialization without deriving serde Serialize/Deserialize trait. Packet::{from_data,populate_packet} methods also only require that the data to be bincode serializable and do not care about serde either. Next version of bincode provides the necessary traits to achieve this functionality: https://docs.rs/bincode/2.0.0-rc.3/bincode/enc/trait.Encode.html but that is not released yet. In order to avoid serde::Serialize trait bound on Packet::{from_data,populate_packet}, the commit adds a new Serialize trait which only requires implementing bincode serialization.

…_packet} As part of anza-xyz#3575 we need to implement bincode (de)serialization without deriving serde Serialize/Deserialize trait. Packet::{from_data,populate_packet} methods also only require that the data to be bincode serializable and do not care about serde either. Next version of bincode provides the necessary traits to achieve this functionality: https://docs.rs/bincode/2.0.0-rc.3/bincode/enc/trait.Encode.html but that is not released yet. In order to avoid serde::Serialize trait bound on Packet::{from_data,populate_packet}, the commit adds a new Encode trait which only requires implementing bincode serialization.

…_packet} (#3636) As part of #3575 we need to implement bincode (de)serialization without deriving serde Serialize/Deserialize traits. Packet::{from_data,populate_packet} methods also only require that the data to be bincode serializable and do not care about serde either. Next version of bincode provides the necessary traits to achieve this functionality: https://docs.rs/bincode/2.0.0-rc.3/bincode/enc/trait.Encode.html but that is not released yet. In order to avoid serde::Serialize trait bound on Packet::{from_data,populate_packet}, the commit adds a new Encode trait which only requires implementing bincode serialization.

#3575 reworks CrdsValue bincode (de)serialization. In order to maintain compatibility and correctness, the commit adds a round trip test for Vec<CrdsValue> (de)serialization and also verifies the serialized bytes against a hard-coded hash.

gregcusack

mostly looks good. just a couple questions and may need to clarify some uses of "raw" numbers

gregcusack · 2024-12-05T22:19:22Z

gossip/src/crds_value.rs

+        bytes: &[u8],
+    ) -> impl Iterator<Item = Result<Self, bincode::Error>> + '_ {
+        // Decode number of items in the slice.
+        let (size, mut bytes) = match convert_fixed_bytes::<[u8; 8], 8>(bytes) {


convert_fixed_bytes::<[u8; 8], 8> what exactly is the last 8 doing here? can we be more descriptive here on what these 8s represent?

size is just the first 8 bytes of the serialized struct? and dictates how long the Vec<CrdsValue> is in terms of items (not bytes)?

Added a comment to clarify.

// bincode::serialize{,_into} serializes sequence length as // a u64 (8 bytes) with fixint encoding in little endian.

gregcusack · 2024-12-05T22:29:53Z

gossip/src/crds_value.rs

+    // Implements bincode::deserialize_from for CrdsValue.
+    pub(crate) fn bincode_deserialize(
+        bytes: &[u8],
+        allow_trailing_bytes: bool,


can you explain trailing bytes? Not sure I understand why we would allow these?

It is the same thing as in bincode:
https://docs.rs/bincode/1.3.3/bincode/config/trait.Options.html#method.allow_trailing_bytes

If you are only deserializing a single CrdsValue then generally you do not want to allow trailing bytes.
But you may be deserializing a struct Foo(CrdsValue, Bar) or a Vec<CrdsValue> in which case the trailing bytes are the next value to be deserialized, so you would want to allow trailing bytes.

gregcusack · 2024-12-05T22:38:13Z

gossip/src/protocol.rs

+                .reject_trailing_bytes()
+                .deserialize(bytes)
+        }
+        let (tag, bytes) = convert_fixed_bytes::<[u8; 4], 4>(bytes)?;


same here, can you elaborate on what the 4s are here?

oh so get the first 4 bytes of the serialized value and convert that into a "tag" which just represents the type of message received (Pull Request, Push Message, etc)? May be worth just clarifying the "raw" numbers here

Added a comment.

// bincode::serialize{,_into} serializes enum tag as a u32 (4 bytes) // with fixint encoding in little endian.

Gossip CrdsValues are deserialized when received as a gossip push message or pull response, but serialized again immediately to obtain a value hash during Crds::insert and then again serialized repeatedly every time the value is pushed to another node or returned as a response to a pull request. In order to avoid repeated serialization of a CrdsValue during its lifetime, the commit manually implements bincode (de)serialization of CrdsValue to hold on to bincode serialized bytes of CrdsData and reuses that for serializing CrdsValue.

gregcusack

how worried are we about the increased memory usage here? almost 2Xing memory cost of a CrdsValue. I don't know the memory profile of agave. But looks like we're getting ~30% decrease in process_gossip_packets_time which is great so probably worth the increase in memory

behzadnouri · 2024-12-11T15:34:42Z

how worried are we about the increased memory usage here? almost 2Xing memory cost of a CrdsValue.

yes, that is one of the concerns with this.
I am thinking of some more changes to the gossip code.
I am holding this off until those changes are in and then re-evaluate.

gregcusack · 2024-12-11T17:07:02Z

how worried are we about the increased memory usage here? almost 2Xing memory cost of a CrdsValue.

yes, that is one of the concerns with this. I am thinking of some more changes to the gossip code. I am holding this off until those changes are in and then re-evaluate.

cool sounds good

behzadnouri force-pushed the cache-crds-ser branch 2 times, most recently from bb477ad to 87eded2 Compare November 13, 2024 13:54

behzadnouri mentioned this pull request Nov 13, 2024

adds {CrdsValue,Protocol}::bincode_serialized_size #3609

Merged

behzadnouri force-pushed the cache-crds-ser branch 2 times, most recently from 330df94 to 2132bae Compare November 13, 2024 18:32

behzadnouri mentioned this pull request Nov 14, 2024

removes serde::Serialize trait bound from Packet::{from_data,populate_packet} #3636

Merged

behzadnouri force-pushed the cache-crds-ser branch from 2132bae to 9536dc8 Compare November 16, 2024 19:56

behzadnouri force-pushed the cache-crds-ser branch from 9536dc8 to 469a6b3 Compare November 18, 2024 18:32

behzadnouri force-pushed the cache-crds-ser branch 2 times, most recently from 58fb1e1 to 5217011 Compare November 18, 2024 18:37

behzadnouri force-pushed the cache-crds-ser branch 4 times, most recently from 860442e to 0028ac6 Compare November 19, 2024 22:17

behzadnouri mentioned this pull request Nov 19, 2024

tests that Vec<CrdsValue> (de)serialization round trips #3710

Merged

behzadnouri force-pushed the cache-crds-ser branch 2 times, most recently from 610f57a to 8186746 Compare November 20, 2024 17:22

behzadnouri force-pushed the cache-crds-ser branch 4 times, most recently from 06e60f8 to 945cbc7 Compare December 4, 2024 18:11

behzadnouri requested a review from gregcusack December 4, 2024 21:32

gregcusack reviewed Dec 5, 2024

View reviewed changes

behzadnouri force-pushed the cache-crds-ser branch from 945cbc7 to 5ddd8c6 Compare December 6, 2024 14:32

behzadnouri requested a review from gregcusack December 6, 2024 14:32

gregcusack reviewed Dec 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

removes repeated bincode serialization of gossip CrdsValues #3575

removes repeated bincode serialization of gossip CrdsValues #3575

behzadnouri commented Nov 11, 2024

behzadnouri commented Nov 13, 2024

gregcusack left a comment

gregcusack Dec 5, 2024

gregcusack Dec 5, 2024

behzadnouri Dec 6, 2024

gregcusack Dec 5, 2024

behzadnouri Dec 6, 2024

gregcusack Dec 5, 2024

gregcusack Dec 5, 2024

behzadnouri Dec 6, 2024

gregcusack left a comment

behzadnouri commented Dec 11, 2024

gregcusack commented Dec 11, 2024

removes repeated bincode serialization of gossip CrdsValues #3575

Are you sure you want to change the base?

removes repeated bincode serialization of gossip CrdsValues #3575

Conversation

behzadnouri commented Nov 11, 2024

Problem

Summary of Changes

behzadnouri commented Nov 13, 2024

gregcusack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregcusack left a comment

Choose a reason for hiding this comment

behzadnouri commented Dec 11, 2024

gregcusack commented Dec 11, 2024