2xTx: Refactor to separate Transaction packets from MTU packets by joncinque · Pull Request #29055 · solana-labs/solana

joncinque · 2022-12-02T22:04:08Z

Problem

As we march towards larger transaction sizes, we want to avoid having a negative impact on the rest of the networking layer. Currently, everything is treated as a general PacketBatch, which is always assumed to have the same size.

For transactions, packets may be larger than 1 MTU, so we need a way to separate transaction packets from all other packets.

Summary of Changes

This is entirely a (pretty big) refactor to make PacketBatch into a generic struct that can accept either a Packet or a TransactionPacket, through a BasePacket trait.

In this PR, both packet types have the same size, so this PR can be merged. Here's what it does in total

create BasePacket trait, implemented by Packet and TransactionPacket
change PacketBatch to Batch<P: BasePacket>, use everywhere
use TransactionPacket for transactions in RPC, banking stage, fetch stage, sigverify, and QUIC

Additional changes required to the refactor:

removed the SigVerifier trait, which only had one other totally unused implementation. This made the templating much less messy
the CUDA sig verifier used to take in *const Packet, while the underlying libs take uint8_t*, so updated that to *const u8. This worked fine in testing.
fetch stage now has two recyclers, one for Packets and one for TransactionPackets. I used the same values to create both of them, but that can be tweaked if needed
in banking stage, separated the creation of banking threads to accommodate the different types. No change to functionality
create two different TransactionSigVerifiers, one for TransactionPacket (normal tx processing) and one for Packet (vote processing). No change to functionality, just might be confusing

The only potential impact to performance would be going through the trait to get the meta() and meta_mut(), but those should get inlined by the compiler since there's no dynamic dispatch, only templating.

For testing, we can add a simple patch to this that doubles TransactionPacket's size.

For future work, the idea is to update TransactionPacket to have variable size.

Fixes #

steviez

This looks like a great step in the right direction; still working through some pieces / digesting so will take a more thorough pass in tomorrow

KirillLykov

Looks good to me: despite the size of the changes, 99% of the them are to introduce parametrized types for packets. For the rest, I have only minor comments

KirillLykov · 2022-12-05T15:08:39Z

-                            &bank_forks,
-                            unprocessed_transaction_storage,
+
+                if i == 0 || i == 1 {


Could you leave a comment why for 0 and 1 we handle it differently?

Looks like this piece of code requires some refactoring: I think this building logic should be moved to constructor (if it is generic enough) or to some factory function. If you agree, would be nice to leave a comment like TODO(jon): #<issue id> refactor unprocessed_transaction_storage creation in banking stage and you can assign it to me

Please don't add new TODOs into the code 🙏🏼

I wanted to keep the changeset as limited as possible while still doing what was needed. I added a point in the project to address this, and I'll put in a comment explaining what's happening.

Please don't add new TODOs into the code 🙏🏼

sorry, I wrote this before the discussion on discord

Pull request has been modified.

joncinque · 2022-12-06T12:24:44Z

Rebased onto #29092

behzadnouri · 2022-12-07T14:33:08Z

+pub trait BasePacket: Default + Clone + Sync + Send + Eq + fmt::Debug {
+    const DATA_SIZE: usize;
+    fn populate_packet<T: Serialize>(&mut self, dest: Option<&SocketAddr>, data: &T) -> Result<()>;


What is the benefit of this trait as opposed to using const generics for packet data size and then defining some typedefs for Packet and TransactionPacket?
Something like:

struct GenericPacket<const N: usize> { buffer: [u8; N], meta: Meta, } type Packet = GenericPacket<PACKET_DATA_SIZE>; type TransactionPacket = GenericPacket<TRANSACTION_DATA_SIZE>;

To be honest, I was worried that a second template parameter would make the refactor even longer and more confusing. But it's much better than a macro! Done in ddca0da, thanks for the suggestion!

Ok, now it's actually done, along with the removal of meta() and meta_mut()

ryoqun · 2022-12-10T14:02:44Z

@joncinque hey, fyi, I'm touching PacketBatch for completely orthogonal functionality addition #29196 than your pr's intention. however, it's conflicting with this pr in purely source code-wise. ;) So, later merge of us will have to fix bunch of merge-conflicts, i guess.

ryoqun · 2022-12-10T14:07:31Z

btw, completely layman's dumb question: why are we so insisting on arrays? is there justifiable perf observation with realistic mainnet-beta load? I think we can just switch to Vec for now, considering the 2xTx project.

behzadnouri · 2022-12-10T21:16:29Z

btw, completely layman's dumb question: why are we so insisting on arrays? is there justifiable perf observation with realistic mainnet-beta load? I think we can just switch to Vec for now, considering the 2xTx project.

I would not suggest using Vec. With vectors we would lose any type-safety checks that the buffer is the right size; and that can introduce subtle bugs in so many stages of pipelines.

joncinque · 2022-12-12T20:30:31Z

hey, fyi, I'm touching PacketBatch for completely orthogonal functionality addition #29196 than your pr's intention

@ryoqun ok great, thanks for the heads up! Since you're planning to backport, it probably makes more sense for you to merge first. It doesn't look the change for me will be too bad, but I guess we'll have to see.

KirillLykov

looks a bit more cumbersome than before but if it since it allows doing static checks, I'm for it

Pull request has been modified.

mvines · 2023-01-03T22:36:29Z

@joncinque - this PR got stale 😭

joncinque · 2023-01-03T23:13:39Z

@mvines we're still not sure if this is the right approach, and need to do some testing on it to see. Thanks for flagging that it went stale!

mvines · 2023-02-20T08:10:24Z

sonofa

joncinque · 2023-02-20T22:15:24Z

Merge conflicts on this were monstrous, so I ended up squashing everything and retrying from there. I've almost got a branch ready, which I'll put up in a new PR

joncinque requested review from KirillLykov, lijunwangs and steviez December 2, 2022 22:04

github-actions Bot added explorer Related to the Solana Explorer webapp web3.js Related to the JavaScript client labels Dec 2, 2022

joncinque force-pushed the twoxtx-base branch from dc573ce to a9e1065 Compare December 2, 2022 23:22

github-actions Bot removed web3.js Related to the JavaScript client explorer Related to the Solana Explorer webapp labels Dec 2, 2022

steviez reviewed Dec 5, 2022

View reviewed changes

Comment thread perf/src/perf_libs.rs

Comment thread sdk/src/packet.rs Outdated

KirillLykov previously approved these changes Dec 5, 2022

View reviewed changes

lijunwangs reviewed Dec 5, 2022

View reviewed changes

Comment thread sdk/src/packet.rs Outdated

lijunwangs reviewed Dec 5, 2022

View reviewed changes

Comment thread streamer/src/nonblocking/quic.rs Outdated

This was referenced Dec 5, 2022

perf: Change underlying Elems to *const u8 #29089

Merged

sdk: Make Packet::meta private, use accessor functions #29092

Merged

joncinque self-assigned this Dec 6, 2022

joncinque force-pushed the twoxtx-base branch from 607aa5e to f1c2e41 Compare December 6, 2022 12:23

behzadnouri reviewed Dec 7, 2022

View reviewed changes

joncinque force-pushed the twoxtx-base branch from 416817c to db2eee1 Compare December 13, 2022 01:51

KirillLykov previously approved these changes Dec 14, 2022

View reviewed changes

joncinque mentioned this pull request Dec 17, 2022

DO NOT MERGE 2xTx: Double transaction size on top of #29055 #29310

Closed

joncinque force-pushed the twoxtx-base branch from 5546317 to 753975d Compare December 19, 2022 17:49

github-actions Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 3, 2023

joncinque removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 3, 2023

joncinque and others added 11 commits January 12, 2023 20:54

twoxtx: Add new constant for max transaction size and use it

400dc1e

Add BasePacket trait and use it almost everywhere

92e592a

Separate Packet and TransactionPacket in core

db6768e

Pass a *const u8 to the perf libs rather than a Packet cast

e267db5

Add comment about building banking threads

60afbd1

Use const generic for packet

54dc2ee

Switch to const generics everywhere

2e435ff

Remove meta() and meta_mut() accessors

d852a9e

Fix client targets

ea19961

Rename back to *PacketBatch* everywhere

7a27de6

Fix merge issues

41653fe

joncinque force-pushed the twoxtx-base branch from 753975d to 41653fe Compare January 12, 2023 20:27

steviez mentioned this pull request Jan 24, 2023

WIP: Introduce PacketBatch with Growable buffer #29859

Closed

github-actions Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 27, 2023

steviez removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 30, 2023

github-actions Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Feb 13, 2023

github-actions Bot closed this Feb 20, 2023

Conversation

joncinque commented Dec 2, 2022

Problem

Summary of Changes

Uh oh!

steviez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

KirillLykov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KirillLykov Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

mvines Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

joncinque Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

KirillLykov Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joncinque commented Dec 6, 2022

Uh oh!

behzadnouri Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

joncinque Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

joncinque Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

ryoqun commented Dec 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryoqun commented Dec 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

behzadnouri commented Dec 10, 2022

Uh oh!

joncinque commented Dec 12, 2022

Uh oh!

KirillLykov left a comment

Choose a reason for hiding this comment

Uh oh!

mvines commented Jan 3, 2023

Uh oh!

joncinque commented Jan 3, 2023

Uh oh!

mvines commented Feb 20, 2023

Uh oh!

joncinque commented Feb 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ryoqun commented Dec 10, 2022 •

edited

Loading

ryoqun commented Dec 10, 2022 •

edited

Loading