Scheduler Messages #30976

apfitzge · 2023-03-29T21:48:09Z

Problem

Banking Scheduler and Worker threads need to communicate via message passing.
Scheduler PR and Worker PR will both use these messages, so it makes sense to separate them into a separate PR to be merged before either scheduler/worker.

Summary of Changes

Messages that are passed from [scheduler to worker] and [worker to scheduler].

The message flow between Scheduler and Worker(s) is as follows:

flowchart LR

    subgraph Forwarding
        direction LR
        C[Scheduler];
        D[Worker];
        C -->|ForwardWork|D -->|FinishedForwardWork|C;
    end
    
    subgraph Consuming
        direction LR
        A[Scheduler];
        B[Worker];
        A -->|ConsumeWork|B -->|FinishedConsumeWork|A;
    end

Fixes #

codecov · 2023-03-30T00:26:40Z

Codecov Report

Merging #30976 (61fb9aa) into master (b53656b) will decrease coverage by 0.1%.
The diff coverage is 0.0%.

@@            Coverage Diff            @@
##           master   #30976     +/-   ##
=========================================
- Coverage    81.5%    81.5%   -0.1%     
=========================================
  Files         727      728      +1     
  Lines      205118   205128     +10     
=========================================
- Hits       167256   167241     -15     
- Misses      37862    37887     +25

core/src/banking_stage/scheduler_messages.rs

apfitzge · 2023-04-11T18:22:56Z

Left a few notes about hesitations I had while cleaning this up. Any thoughts would be appreciated 😄

core/src/banking_stage/scheduler_messages.rs

tao-stones · 2023-04-13T01:37:53Z

core/src/banking_stage/scheduler_messages.rs

+/// Message: [Worker -> Scheduler]
+/// Processed transactions.
+pub struct FinishedConsumeWork {
+    pub work: ConsumeWork,


does this make copy of Vec<SanitizedTransaction> and send back, results 2X transactions being send back-n-forth between scheduler and worker? If so, that could become a hot spot, given number of transactions should only increase in the future.

We only need references to the transactions to process them (relevant worker code), so there's no need to take them out of the ConsumeWork struct

In the implementation (linked above), the work field here, is the original ConsumeWork received by the worker, no cloning, which I think is what you are concerned about?

To be entirely clear/pedantic, it will do a copy of the struct when we move it into the Finished* wrappers. But that's just a shallow-copy of the vecs, so the entire struct copy is only 64 bytes. We could reduce that to 8 bytes by wrapping it in a Box, but then we have 2 pointer follows to get to the txs; not sure it'd be worth it. @ryoqun might have some thoughts here 😄

🤔

So after writing the above comment, I was thinking "how can we enforce the fact the txs aren't cloned out by worker".

Maybe it's a better interface to not pub the fields, and add some impls for the messages:

fn new() -> Self;

fn transactions(&self) -> &[SanitizedTransaction];

fn finish(retryable_indexes: Vec<usize>) -> FinishConsumeWork;

and similarly for the ForwardWork message pair.

@taozhu-chicago, @ryoqun - do you think this is a better interface than what I've got here with the pub fields?

the work field here, is the original ConsumeWork received by the worker, no cloning, which I think is what you are concerned about?

Yep, without see the actual implementation, I wasn't able to tell is work: Vec<SanitizedTransaction> is moved or copied. So yeah, would be a good idea to make message interface enforce ref to avoid accidental copy.

🤔
Thinking the interface I described above won't be sufficient to enforce that, since we need to get the values out on the finished side. Will think about a better design tonight.

Also back to my original comment, it's probably slightly misleading. We do have to copy (move not clone - so still not copying the underlying Vecs within the tx), the memory of the SanitizedTransaction from the Scheduler's storage into the Vec when we create the batch. And back from Vec into Scheduler storage iff it is returned as retryable.

Struggling to come up with a good way to enforce this in rust. We still want the fields of FinishedConsumeWork to be pub so we can pull them out at the end, but also want to restrict construction, to come from an existing ConsumeWork.

Potentially something like this:

// needed so clippy doesn't yell about using #[non_exhaustive], // which only restricts OUTSIDE the crate... #[allow(clippy::manual_non_exhaustive)] pub struct FinishedConsumeWork { pub work: ConsumeWork, pub retryable_indexes: Vec<usize>, _priv: (), } impl FinishedConsumeWork { fn new(work: ConsumeWork, retryable_indexes: Vec<usize>) -> Self { Self { work, retryable_indexes, _priv: (), } } }

but I think that's fairly ugly on the receiving side.

After considering it more, I'm hesitant to put any implementation in what are supposed to be simple messages. wdyt @taozhu-chicago / @ryoqun ?

yea that does feel like bit of over engineering, at least at this stage. I am cool with committing what's ready to move forward.

yeah, i think current form is fine as-is.

core/src/banking_stage/scheduler_messages.rs

tao-stones

lgtm

core/src/banking_stage/scheduler_messages.rs

apfitzge · 2023-04-18T13:27:45Z

core/src/banking_stage/scheduler_messages.rs

+    pub batch_id: TransactionBatchId,
+    pub transaction_ids: Vec<TransactionId>,
+    pub transactions: Vec<SanitizedTransaction>,
+    pub max_age_slots: Vec<Slot>,


following a discussion with @ryoqun, we shouldn't let SanitizedTransactions pass epoch boundaries - so we can mark txs (at sanitize time) with a max age slot it will expire at.

Workers should check this before execution, and quickly disregard txs if the bank the worker is using > max_age_slot

ryoqun · 2023-04-19T05:05:48Z

core/src/banking_stage/scheduler_messages.rs

+}
+
+/// Message: [Scheduler -> Worker]
+/// Transactions to be consumed (executed, recorded, committed)


nit: Transactions to be consumed (i.e. executed, recorded, and committed)

done: eb516c5 + 61fb9aa

ryoqun · 2023-04-19T05:09:18Z

core/src/banking_stage/scheduler_messages.rs

+/// Transactions to be consumed (executed, recorded, committed)
+pub struct ConsumeWork {
+    pub batch_id: TransactionBatchId,
+    pub transaction_ids: Vec<TransactionId>,


naming: ids to be consistent with ForwardWork? (a field with identical type Vec<TransactionId> isn't called as packet_ids there currently).

done: 005ebd9

I originally had ids, then moved away to clarify from batch_id (which I had originally named id...then never changed these back! I think ids is already clear enough, so changed to be consistent between the messages.

ryoqun · 2023-04-19T05:33:11Z

core/src/banking_stage/scheduler_messages.rs

@@ -0,0 +1,54 @@
+use {
+    crate::immutable_deserialized_packet::ImmutableDeserializedPacket,
+    solana_poh::poh_recorder::Slot, solana_sdk::transaction::SanitizedTransaction, std::sync::Arc,


nit: solana_sdk::clock::Slot is more authoritative, iirc.

done: efcff1c

good catch, must have chosen the wrong suggested import from rust-analyzer!

fyi, this won't happen again: #31832

ryoqun

lgtm with nits

tao-stones

re-approve after nits

apfitzge added the work in progress This isn't quite right yet label Mar 29, 2023

apfitzge force-pushed the feature/scheduler_messages branch 2 times, most recently from 4d14eb0 to 8ddc0b1 Compare March 29, 2023 22:45

apfitzge commented Mar 30, 2023

View reviewed changes

core/src/banking_stage/scheduler_messages.rs Outdated Show resolved Hide resolved

apfitzge commented Mar 30, 2023

View reviewed changes

core/src/banking_stage/scheduler_messages.rs Outdated Show resolved Hide resolved

Scheduler Messages

0eb6499

apfitzge force-pushed the feature/scheduler_messages branch from 8ddc0b1 to 0eb6499 Compare April 6, 2023 19:12

apfitzge commented Apr 7, 2023

View reviewed changes

core/src/banking_stage/scheduler_messages.rs Outdated Show resolved Hide resolved

apfitzge removed the work in progress This isn't quite right yet label Apr 11, 2023

apfitzge marked this pull request as ready for review April 11, 2023 18:22

apfitzge requested review from ryoqun and tao-stones April 11, 2023 18:22

This was referenced Apr 11, 2023

Banking worker #30970

Merged

Multi-Iterator Scheduler #31148

Closed

apfitzge commented Apr 12, 2023

View reviewed changes

core/src/banking_stage/scheduler_messages.rs Outdated Show resolved Hide resolved

tao-stones reviewed Apr 13, 2023

View reviewed changes

apfitzge added 2 commits April 13, 2023 21:17

TransactionBatchId - simple u64 wrapper

5fffe13

TransactionId - simple u64 index wrapper

fe5922d

tao-stones previously approved these changes Apr 14, 2023

View reviewed changes

ryoqun reviewed Apr 18, 2023

View reviewed changes

core/src/banking_stage/scheduler_messages.rs Outdated Show resolved Hide resolved

fix typo: reverse worker -> scheduler on ForwardWork

c238f5a

apfitzge dismissed tao-stones’s stale review via c238f5a April 18, 2023 12:23

add max_age_slots to ConsumeWork

7031b1a

apfitzge commented Apr 18, 2023

View reviewed changes

apfitzge requested review from ryoqun and tao-stones April 18, 2023 20:00

ryoqun reviewed Apr 19, 2023

View reviewed changes

ryoqun previously approved these changes Apr 19, 2023

View reviewed changes

apfitzge added 3 commits April 19, 2023 16:50

clarify ConsumeWork message comment

eb516c5

ConsumeWork transaction_ids -> ids

005ebd9

import Slot from sdk instead of poh

efcff1c

apfitzge dismissed ryoqun’s stale review via efcff1c April 19, 2023 16:53

Better clarify to ConsumeWork message comment

61fb9aa

tao-stones approved these changes Apr 19, 2023

View reviewed changes

apfitzge merged commit 7a393e4 into solana-labs:master Apr 19, 2023

ryoqun mentioned this pull request May 26, 2023

Don't needlessly expose Slot from solana-poh #31832

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheduler Messages #30976

Scheduler Messages #30976

apfitzge commented Mar 29, 2023 •

edited

Loading

codecov bot commented Mar 30, 2023 •

edited

Loading

apfitzge commented Apr 11, 2023

tao-stones Apr 13, 2023

apfitzge Apr 13, 2023

apfitzge Apr 13, 2023

tao-stones Apr 13, 2023

apfitzge Apr 13, 2023

apfitzge Apr 14, 2023

tao-stones Apr 14, 2023

ryoqun Apr 19, 2023

tao-stones left a comment

apfitzge Apr 18, 2023

ryoqun Apr 19, 2023

apfitzge Apr 19, 2023

ryoqun Apr 19, 2023

apfitzge Apr 19, 2023

ryoqun Apr 19, 2023

apfitzge Apr 19, 2023

ryoqun May 26, 2023

ryoqun left a comment

tao-stones left a comment

Scheduler Messages #30976

Scheduler Messages #30976

Conversation

apfitzge commented Mar 29, 2023 • edited Loading

Problem

Summary of Changes

codecov bot commented Mar 30, 2023 • edited Loading

Codecov Report

apfitzge commented Apr 11, 2023

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

tao-stones 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

ryoqun left a comment

Choose a reason for hiding this comment

tao-stones left a comment

Choose a reason for hiding this comment

apfitzge commented Mar 29, 2023 •

edited

Loading

codecov bot commented Mar 30, 2023 •

edited

Loading