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

Shreds incur extra copies in several places #1142

Open
steviez opened this issue May 2, 2024 · 3 comments
Open

Shreds incur extra copies in several places #1142

steviez opened this issue May 2, 2024 · 3 comments

Comments

@steviez
Copy link

steviez commented May 2, 2024

Problem

The shred object currently contains owned memory via a Vec<u8>. In several places, shreds are very temporarily created as an intermediate. Given the owned memory, we have extra copies:

1. Shred insertion into Blockstore - copy buffers from a PacketBatch to individual Shreds:

let shred = Shred::new_from_serialized_shred(shred.to_vec()).ok()?;

The shreds have a short, common lifetime, that ends shortly after this copy. One minor exception is for any duplicate shreds that may be found (the duplicates have some further processing).

2. Shred fetch from Blockstore via Blockstore::get_slot_entries_in_block(). UPDATE: this item has been resolved - see #1142 (comment)

Original issue description for shred fetch

Some(pinnable_slice) => Ok(Some(pinnable_slice.as_ref().to_vec())),

let data: Vec<_> = shreds.iter().map(Shred::data).collect::<Result<_, _>>()?;
let data: Vec<_> = data.into_iter().flatten().copied().collect();

agave/ledger/src/blockstore.rs

Lines 3579 to 3584 in 12d009e

.and_then(|payload| {
bincode::deserialize::<Vec<Entry>>(&payload).map_err(|e| {
BlockstoreError::InvalidShredData(Box::new(bincode::ErrorKind::Custom(
format!("could not reconstruct entries: {e:?}"),
)))
})

The copy into common buffer (2nd snippet) and bincode deserialize (3rd snippet) are necessary. However, the initial copy to owned memory (1st snippet) is seemingly not necessary. Note that shred fetch occurs for both ReplayStage and CompletedDataSetsService, so the effects of this one are doubled.

Impact

Looking at MNB metrics:

  1. I see about ~2700 packets / second coming through (8k on recv-window-insert-shreds.num_packets and datapoint reported every 3 seconds)
  2. I see about ~1500 shreds / second (600 for shred_insert_is_full.last_index * 2.5 blocks / second).
    • Again, for below calculation, double this number because of ReplayStage and CompletedDataSetsService

So, in total, this is about ~5700 allocations and ~6.75 MB of memory copy that could be avoided, per second

Proposed Solution

Rework Shred to allow owned or borrowed memory, such as what Cow provides

@steviez
Copy link
Author

steviez commented May 2, 2024

At some point in the near future, I'll gather some data with perf so we can get a better idea of how much the previously mentioned numbers contribute to overall process numbers. Doing so might help us attach some sense of priority (ie if these are 0.01% of all allocations, we might decide we have bigger fish to fry at the moment).

@steviez
Copy link
Author

steviez commented Jan 17, 2025

Looking at MNB metrics:

I see about ~2700 packets / second coming through (8k on recv-window-insert-shreds.num_packets and datapoint reported every 3 seconds)
I see about ~1500 shreds / second (600 for shred_insert_is_full.last_index * 2.5 blocks / second).
Again, for below calculation, double this number because of ReplayStage and CompletedDataSetsService

Minor note - CompletedDataSetsService is an RPC only service and it has been updated to only get spawned for RPC nodes as of #1363 so any solution to the stated problem here may not be as profound for voting (non RPC validators).

But, blocks are bigger in number of shreds (and will presumably continue to get bigger as we increase CU limits) based on recent MNB traffic compared to when I first wrote this up. AND, RPC is core infrastructure so there is absolutely benefit in optimizing for these as well. Lastly, getBlock queries that hit local ledger will see some gains as this query fetches shreds + deserializes into transactions

@steviez
Copy link
Author

steviez commented Jan 28, 2025

Shred fetch from Blockstore via Blockstore::get_slot_entries_in_block()

In regards to this item, data is now copied directly from rocksdb owned memory into the common buffer passed to bincode::deserialize()

So, there is likely not much else to do on this piece.

Shred insertion into Blockstore - copy buffers from a PacketBatch to individual Shreds:

There is potentially still room for optimization on this aspect though. I will update the issue to reflect that 2. is mostly resolved

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

No branches or pull requests

1 participant