Define InstalledScheduler::wait_for_termination() #33922

ryoqun · 2023-10-29T06:40:22Z

Problem

The upcoming unified scheduler is async by nature. so, the replay stage must eventually be blocked on block boundaries in some way for tx execution completion. however, the minimal InstalledSchduler (introduced at #33875) doesn't provide any such way.

Summary of Changes

Implement it and wire it exhaustively into all the needed relevant blocking points.

(fyi, planned next prs: 1. introduce InstallSchedulerPool 2. introduce SchedulerPool as its actual impl)

extracted from: #33070

ryoqun · 2023-10-29T07:12:10Z

runtime/src/bank.rs

@@ -4185,7 +4186,11 @@ impl Bank {
    /// Register a new recent blockhash in the bank's recent blockhash queue. Called when a bank
    /// reaches its max tick height. Can be called by tests to get new blockhashes for transaction
    /// processing without advancing to a new bank slot.
-    pub fn register_recent_blockhash(&self, blockhash: &Hash) {
+    fn register_recent_blockhash(&self, blockhash: &Hash, scheduler: &InstalledSchedulerRwLock) {


pub isn't needed anymore as of #33880..

also, this internal fn should ever be called by register_tick. even more so after this new scheduler arg. in other words, blocking must be done if needed with proper scheduler plumbing.

ryoqun · 2023-10-29T07:16:29Z

runtime/src/installed_scheduler_pool.rs

+                .as_mut()
+                .and_then(|scheduler| scheduler.wait_for_termination(&reason));
+            if !reason.is_paused() {
+                drop(scheduler.take().expect("scheduler after waiting"));


this meaningless drop() and .take() will be replaced with SchedulerPool::return_to_pool() by a later pr.

ryoqun · 2023-10-29T08:05:41Z

runtime/src/bank.rs

+        // This is needed because recent_blockhash updates necessitate synchronizations for
+        // consistent tx check_age handling.
+        BankWithScheduler::wait_for_paused_scheduler(self, scheduler);


this is indeed very subtle yet crucial. there's tests for this: test_scheduler_schedule_execution_recent_blockhash_edge_case_{with,without}_race in #33070

So I'm trying to just summarize the reason this is necessary.

Essentially, in the loop in blcokstore_processor.rs, scheduling no longer blocks our transaction processing. This means we will end up scheduling until we hit the final tick, and which point we register the new bank hash.

We need to make sure the blockhash_queue of our bank is not updated until after all the scheduled transactions have completed, because if a tx w/ recent blockhash equal to the current blockhash was scheduled we'd get inconsistent results based on when it was executed not scheduled.

I think that is correct, but please let me know if it is not.
If that is the case, why did we choose this approach instead of checking blockhash at schedule time instead?
afaik blockhash queue is only updated at the end of a block, i.e. when we should not be scheduling any more txs for that slot.

Essentially, in the loop in blcokstore_processor.rs, scheduling no longer blocks our transaction processing. This means we will end up scheduling until we hit the final tick, and which point we register the new bank hash.

We need to make sure the blockhash_queue of our bank is not updated until after all the scheduled transactions have completed, because if a tx w/ recent blockhash equal to the current blockhash was scheduled we'd get inconsistent results based on when it was executed not scheduled.

that's correct understanding.

If that is the case, why did we choose this approach instead of checking blockhash at schedule time instead?
afaik blockhash queue is only updated at the end of a block, i.e. when we should not be scheduling any more txs for that slot.

there's two reasons:

impl simplify and risk reduction (i.e. extracting Bank::check_age() is too dangerous without some rigourious code audit, which i want to avoid (esp considering nonces). Certainly, this is nice optimization opportunity. maybe future work).

the recent blockhashes are exposed to runtime as a sysvar (SysvarRecentB1ockHashes11111111111111111111) in theory. In theory here means this isn't the case due to the existence of SysvarCache, which is updated only at the start of slot. However, I don't want to rely on some peculiar behavior of the cache not to be fragile. Again, this is fixable, but this will be of (distant) future work).

Alternatively, we can route the recentblockhash update via the scheduler like this:

enum SchedulerTask { ExecuteTransaction(txes...), UpdateRecentBlockhash(hash), }

so that this particular blocking call can be consolidated into replaystage's wait_for_completed_scheduler(). but again, this is out-of-scope for now due to the need of auditing any potential affected access timings to the bank's recent blockhashes and its implications.

codecov · 2023-10-29T08:17:05Z

Codecov Report

Merging #33922 (507c502) into master (cdc2841) will decrease coverage by 0.1%.
The diff coverage is 82.7%.

@@            Coverage Diff            @@
##           master   #33922     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         809      809             
  Lines      218060   218241    +181     
=========================================
+ Hits       178630   178769    +139     
- Misses      39430    39472     +42

runtime/src/installed_scheduler_pool.rs

apfitzge · 2023-10-30T02:21:07Z

runtime/src/installed_scheduler_pool.rs

+pub type ResultWithTimings = (Result<()>, ExecuteTimings);
+
+#[derive(Debug, PartialEq, Eq, Clone, Copy)]
+pub enum WaitReason {


Could you also comment on this from the call-side perspective? It's unclear to me, when I call wait_for_terminate with one of these reasons...is that me telling the scheduler to terminate with that reason, or is it just waiting for the reason to happen?

did my best: 507c502

ryoqun · 2023-10-30T08:05:53Z

@apfitzge thanks for pointing out right questions. hope my newly added in-source comments make your confusion vanish...

apfitzge

Happy with the changes. Tests are very clear and easy to follow with the mocked scheduler behavior.

ryoqun force-pushed the wait-for-termination branch from 6e83529 to 565e7ce Compare October 29, 2023 06:48

Define InstalledScheduler::wait_for_termination()

db50d2c

ryoqun force-pushed the wait-for-termination branch from 565e7ce to db50d2c Compare October 29, 2023 07:01

ryoqun commented Oct 29, 2023

View reviewed changes

ryoqun requested a review from apfitzge October 29, 2023 07:20

ryoqun commented Oct 29, 2023

View reviewed changes

apfitzge reviewed Oct 30, 2023

View reviewed changes

ryoqun added 2 commits October 30, 2023 14:55

Rename to wait_for_scheduler_termination

a1145ea

Comment wait_for_termination and WaitReason better

507c502

ryoqun force-pushed the wait-for-termination branch from ff1a20a to 507c502 Compare October 30, 2023 08:01

ryoqun requested a review from apfitzge October 30, 2023 08:04

apfitzge approved these changes Oct 31, 2023

View reviewed changes

ryoqun merged commit 136ab21 into solana-labs:master Oct 31, 2023
43 checks passed

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore anza-xyz/agave#475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define InstalledScheduler::wait_for_termination() #33922

Define InstalledScheduler::wait_for_termination() #33922

ryoqun commented Oct 29, 2023 •

edited

Loading

ryoqun Oct 29, 2023 •

edited

Loading

ryoqun Oct 29, 2023 •

edited

Loading

ryoqun Oct 29, 2023 •

edited

Loading

apfitzge Oct 31, 2023

ryoqun Oct 31, 2023 •

edited

Loading

codecov bot commented Oct 29, 2023 •

edited

Loading

apfitzge Oct 30, 2023

ryoqun Oct 30, 2023

ryoqun commented Oct 30, 2023 •

edited

Loading

apfitzge left a comment

Define InstalledScheduler::wait_for_termination() #33922

Define InstalledScheduler::wait_for_termination() #33922

Conversation

ryoqun commented Oct 29, 2023 • edited Loading

Problem

Summary of Changes

ryoqun Oct 29, 2023 • edited Loading

Choose a reason for hiding this comment

ryoqun Oct 29, 2023 • edited Loading

Choose a reason for hiding this comment

ryoqun Oct 29, 2023 • edited Loading

Choose a reason for hiding this comment

apfitzge Oct 31, 2023

Choose a reason for hiding this comment

ryoqun Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 29, 2023 • edited Loading

Codecov Report

apfitzge Oct 30, 2023

Choose a reason for hiding this comment

ryoqun Oct 30, 2023

Choose a reason for hiding this comment

ryoqun commented Oct 30, 2023 • edited Loading

apfitzge left a comment

Choose a reason for hiding this comment

ryoqun commented Oct 29, 2023 •

edited

Loading

ryoqun Oct 29, 2023 •

edited

Loading

ryoqun Oct 29, 2023 •

edited

Loading

ryoqun Oct 29, 2023 •

edited

Loading

ryoqun Oct 31, 2023 •

edited

Loading

codecov bot commented Oct 29, 2023 •

edited

Loading

ryoqun commented Oct 30, 2023 •

edited

Loading