Apply cleanups to solana-core for unified scheduler #4123
Conversation
ryoqun
force-pushed
the
unified-scheduler-core-cleanups
branch
from
f6fc326 to
f7c63e0
Compare
ryoqun
changed the title
ryoqun
changed the title
| @@ -803,11 +807,25 @@ impl BankingStage { | |||
| } | |||
| } | |||
|
|
|||
| #[cfg_attr(feature = "dev-context-only-utils", qualifiers(pub))] | |||
| pub(crate) fn update_bank_forks_and_poh_recorder_for_new_tpu_bank( | |||
There was a problem hiding this comment.
refer to https://github.com/anza-xyz/agave/pull/3946/files#r1886311350 (or #3946 (comment)) for justification of this change.
| @@ -1409,6 +1410,7 @@ impl Validator { | |||
| let cluster_slots = | |||
| Arc::new(crate::cluster_slots_service::cluster_slots::ClusterSlots::default()); | |||
|
|
|||
| let root_bank_cache = RootBankCache::new(bank_forks.clone()); | |||
There was a problem hiding this comment.
refer to https://github.com/anza-xyz/agave/pull/3946/files#r1886312626 (or #3946 (comment)) for justification of this movement.
| pub type BankingPacketBatch = Arc<Vec<PacketBatch>>; | ||
| pub type BankingPacketReceiver = Receiver<BankingPacketBatch>; |
There was a problem hiding this comment.
these types are planned to be used by solana-unified-scheduler-pool crate, so they need to be moved higher in the dep graph from solana-core.
| poh_recorder: &RwLock<PohRecorder>, | ||
| tpu_bank: Bank, | ||
| track_transaction_indexes: bool, | ||
| ) { |
There was a problem hiding this comment.
not returning BankWithScheduler is intentional because production code (i.e. relevant ReplayingStage) doesn't need it, while making dev call-sites more verbose a bit.
ryoqun
force-pushed
the
unified-scheduler-core-cleanups
branch
from
f7c63e0 to
aabd2ac
Compare
| @@ -13,6 +13,7 @@ edition = { workspace = true } | |||
| ahash = { workspace = true } | |||
| bincode = { workspace = true } | |||
| bv = { workspace = true, features = ["serde"] } | |||
| crossbeam-channel = { workspace = true } | |||
There was a problem hiding this comment.
kind of arbitrary extra dep addition, but i think it's okay for better code organization. alternatively, we can define BankingPacketBatch/BankingPacketReceiver at solana-unified-scheduler-pool without a single dep graph chagne, but it felt me that it's off-place.
There was a problem hiding this comment.
Not a big fan of putting it here. perf seems isolated from any communication, so doesn't seem correct to me to add this just for a type-alias.
Maybe now is a good time to just set up a more (imo) reasonable structure of crates? (doesn't matter THAT much since both banking and sigverify are in core)
A typical pattern that I've seen for crates is something like this:
/
├── stage1/
├── communication/
└── stage2/
The communication crate is separate from both the upstream and downstream consumer(s), and they both just need to follow the interface defined in communication. Think it is a step toward separating sigverify & banking code from core as well.
There was a problem hiding this comment.
I pondered on naming for awhile. how about solana-core-types as the name of the communication crate?
I acknowledge the awkwardness of putting these into solana-perf and I understand the appeal of cleaner crate organization you suggested.
that said, I think that crate will be very small and kind of limited, in practice. say, we can't put the sibling type alias BankingPacketSender into that crate, because that would pull some impls into the crate (i.e. TracedSender). so, the crate will contain strictly some common set of type aliases basically.
so, i named the crate as -type with that in mind.
if it's really okay for you, i'll create such a crate in real quick.
There was a problem hiding this comment.
These would be specific to agave client, so agave- prefix is more appropriate for naming imo.
If it's just going to be communication between SigVerifyStage (possibly soon to be renamed VerifyStage) and BankingStage (should we rename this to BlockProductionStage?) I wonder if the name core-types is too generic - there's a LOT of other stuff in core
There was a problem hiding this comment.
hmm, how about agave-banking-stage-types, then? i thought we won't rename replaying-stage to block-verification-stage, so picked the banking-stage likewise
|
@apfitzge thanks for quick review. Other than #4123 (comment), is there other concerns? If other changes are mergeable, I'll back-off the commit and create another pr for it, while landing other changes in this pr, if possible.. |
Still looking, have some other stuff need to review this morning, and didn't get a chance to finish the review of this one yet |
Problem
There are various small changes to support block production by unified scheduler.
Summary of Changes
Do them at once in a single pr, rather than 6 different prs.
Commits are well logically separated.
No functional change, so no test is added.
extracted from #3946, see the pr for the general overview.