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

generic scheduler #3611

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

apfitzge
Copy link

Problem

  • scheduler doesn't allow us to handle non SanitizedTransaction types
  • we must kill SanitizedTransaction for performance

Summary of Changes

  • abstract receiving and buffering in the scheduler controller and all downstream components

Fixes #

Comment on lines 634 to 640
let packet_deserializer = PacketDeserializer::new(non_vote_receiver);
let receive_and_buffer = SanitizedTransactionReceiveAndBuffer::new(
packet_deserializer,
bank_forks.clone(),
forwarder.is_some(),
);
let container = TransactionStateContainer::with_capacity(TOTAL_BUFFERED_PACKETS);
Copy link
Author

Choose a reason for hiding this comment

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

Once the other structures/impl for new transaction type are ready there will be a switch here on spawning.

  • Receiving and buffering is generic so that we can skip over all deserialization (never pass through ImmutableDeserializedPacket)
  • Container is generic because the current impl isn't good if we want to copy as little data as possible. For new tx types, it will wrap the old type with a pre-allocated (separate) container for getting free Bytes spaces to copy our packet data into. Eventually this would go away once we have Bytes upstream, but for now we don't and this is much faster than allocating space or copying a Packet for every tx move.

@apfitzge apfitzge marked this pull request as ready for review November 13, 2024 20:04
jstarry
jstarry previously approved these changes Nov 14, 2024
@@ -44,7 +44,7 @@ pub trait ReceiveAndBuffer<Tx: TransactionWithMeta> {
) -> bool;
}

pub(crate) struct SanitizedTransactionReceiveAndBuffer {
Copy link

Choose a reason for hiding this comment

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

nit: don't think visibility change was necessary

Comment on lines 37 to 41
pub(crate) struct SchedulerController<
C: LikeClusterInfo,
Tx: TransactionWithMeta,
R: ReceiveAndBuffer<Tx>,
> {
Copy link

Choose a reason for hiding this comment

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

Idk I prefer keeping the Tx generic parameter personally. Feels backwards to be getting it from the ReceiveAndBuffer rather than passing it

Copy link
Author

Choose a reason for hiding this comment

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

Idea I had here is that ReceiveAndBuffer's type(s) are a single-point controlling all the inner implementations.

I also think this sort of set up is somewhat necessary with what I have planned.
(See most recent push, Container type is also chosen by the ReceiveAndBuffer).

Old transaction type:

  • receive packet batches, deserialize into ImmutableDeserializedPacket
  • do filtering, eventually end up with RuntimeTransaction<SanitizedTransaction>
  • insert those into the TransactionStateContainer<RuntimeTransaction<SanitizedTransaction>>

New transaction type:

  • receive packet batches. get pre-allocated space (BytesMut) from the container, copy bytes of packet, parse (using Bytes).
  • do filtering, eventually end up with RuntimeTransaction<ResolvedTransactionView>
  • insert those into the the a different container type (WrappedTransactionStateContainer).
    • this wrapping is necessary so that we can get and re-use the Bytes for the transactions

So it doesn't make sense if we have the TransactionViewReceiveAndBuffer and a TransactionStateContainer<..>, because we need slightly different behavior due to the type differences; and they require a slightly different interface as well, since getting free space to work on received packets isn't something the base TransactionStateContainer needs to do.

A lot of this gets simplified once we are receiving Bytes from upstream, because we could just insert those into the TransactionStateContainer. But that's blocked rn because of CUDA stuff (that no one uses) 🫠

Copy link
Author

Choose a reason for hiding this comment

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

^ kind of a long winded explanation. let me know if you want to hop on a call to discuss, or if you have questions here it's fine too!

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

Successfully merging this pull request may close these issues.

2 participants