chore(concurrency): sleep if there are no tasks

[Yoni-Starkware]

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.45%. Comparing base (c3267c0) to head (db34c50).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1991   +/-   ##
=======================================
  Coverage   78.45%   78.45%           
=======================================
  Files          62       62           
  Lines        8970     8970           
  Branches     8970     8970           
=======================================
  Hits         7037     7037           
  Misses       1486     1486           
  Partials      447      447           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[avi-starkware]

I don't like using None. It is vague, and it messes up some of the types. When do we want it to be None and when Task::NoTask? Why not use something descriptive?

I propose adding Task::Wait. Task::Wait will trigger the sleep, and Task::NoTask will immediately ask for another task.

[avi-starkware]

crates/blockifier/src/concurrency/scheduler_test.rs line 219 at r1 (raw file):

#[case::returns_execution_task(0, 10, true)]
#[case::does_not_return_execution_task(10, 0, true)]
fn test_finish_validation(

I renamed the method but forgot to rename the corresponding test.

Suggestion:

fn test_finish_abort(

[avi-starkware]

crates/blockifier/src/concurrency/scheduler_test.rs line 247 at r1 (raw file):

            assert_eq!(*new_status, TransactionStatus::Executed);
        }
    }

Suggestion:

    let scheduler =
        default_scheduler!(chunk_size: DEFAULT_CHUNK_SIZE, execution_index: execution_index);
    scheduler.set_tx_status(tx_index, TransactionStatus::Executed);
    let mut result = None;
    result = scheduler.finish_abort(tx_index);
    let new_status = scheduler.lock_tx_status(tx_index);
    if execution_index > tx_index {
        assert_eq!(result, Some(Task::ExecutionTask(tx_index)));
        assert_eq!(*new_status, TransactionStatus::Executing);
    } else {
        assert!(result.is_none());
        assert_eq!(*new_status, TransactionStatus::ReadyToExecute);
    }

[avi-starkware]

crates/blockifier/src/concurrency/scheduler_test.rs line 247 at r1 (raw file):

            assert_eq!(*new_status, TransactionStatus::Executed);
        }
    }

Oops, the status should be TransactionStatus::Aborting.

[avi-starkware]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)

---

crates/blockifier/src/concurrency/scheduler_test.rs line 216 at r1 (raw file):

#[rstest]
#[case::not_aborted(0, 10, false)]

We no longer have the not aborted flow inside this method (if it's not aborted we simply don't call the method).

Here's a PR that fixes this test, in case you don't want all these changes in your PR:
#1993

---

crates/blockifier/src/concurrency/scheduler_test.rs line 219 at r1 (raw file):

Previously, avi-starkware wrote…

I renamed the method but forgot to rename the corresponding test.

Here's a PR that fixes this test, in case you don't want all these changes in your PR:
#1993

---

crates/blockifier/src/concurrency/scheduler_test.rs line 222 at r1 (raw file):

    #[case] tx_index: TxIndex,
    #[case] execution_index: TxIndex,
    #[case] aborted: bool,

We no longer have the not aborted flow inside this method (if it's not aborted we simply don't call the method).

Here's a PR that fixes this test, in case you don't want all these changes in your PR:
#1993

---

crates/blockifier/src/concurrency/scheduler_test.rs line 247 at r1 (raw file):

Previously, avi-starkware wrote…

Oops, the status should be TransactionStatus::Aborting.

Here's a PR that fixes this test, in case you don't want all these changes in your PR:
#1993

[Yoni-Starkware]

None means that you're not holding any task and you need to query the scheduler.
NoTask means that there are no pending tasks at all (and you can go to sleep).
I needed to distinguish between these cases because I didn't want to sleep, for example, after every execute.

Not sure I like Wait, it's too specific - it describes what you do if there are no tasks. I can do nothing or call spin_loop for example

But I agree it's not ideal.I'll think

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @noaov1)

[Yoni-Starkware]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

crates/blockifier/src/concurrency/scheduler_test.rs line 216 at r1 (raw file):

Previously, avi-starkware wrote…

We no longer have the not aborted flow inside this method (if it's not aborted we simply don't call the method).

Here's a PR that fixes this test, in case you don't want all these changes in your PR:
#1993

Done.

---

crates/blockifier/src/concurrency/scheduler_test.rs line 219 at r1 (raw file):

Previously, avi-starkware wrote…

Here's a PR that fixes this test, in case you don't want all these changes in your PR:
#1993

Done.

---

crates/blockifier/src/concurrency/scheduler_test.rs line 222 at r1 (raw file):

Previously, avi-starkware wrote…

We no longer have the not aborted flow inside this method (if it's not aborted we simply don't call the method).

Here's a PR that fixes this test, in case you don't want all these changes in your PR:
#1993

Done.

---

crates/blockifier/src/concurrency/scheduler_test.rs line 247 at r1 (raw file):

Previously, avi-starkware wrote…

Here's a PR that fixes this test, in case you don't want all these changes in your PR:
#1993

Done.

[Yoni-Starkware]

@avi-starkware I changed it a bit, WDYT?

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @noaov1)

[avi-starkware]

I understood your usage of None and Task::NoTask after looking at how you use it in the code. My question was rhetorical - I meant to say that someone seeing this code for the first time will have a hard time understanding the distinction between the two.

[Yoni-Starkware]

finish_abort can only return a re-execution task, so using option there is not so bad IMO
I can also change validate to return a boolean, something like this

                    if let Some(re_execute_task) = self.validate(tx_index) {
                        task = re_execute_task;
                        continue;
                    }

==>>

                    let should_re_execute = self.validate(tx_index) {
                    if should_re_execute {
                        task = Task::ExecuteTask(tx_index);
                        continue;
                    }

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @noaov1)

[avi-starkware]

How about something like:
1.Task::GetNextTask/ Task::AskForTask / Task::GetTask / Task::Empty
2. Task::NoTaskAvailable / Task::NoneAvailable / Task::Pending
This will make it clear what each is supposed to mean, with no prescribed wait.
I like it when all options are enum members, because it makes it so that each member clearly occupies a single match arm. With None, the flow of a match arm splits into cases, making it less clean.

Also, note that when we add dependencies, execute will also return a task in some cases, making the if let Some appear twice in the worker loop...

[Yoni-Starkware]

Good point, thanks.
Done (cc @noaov1)

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @noaov1)

[avi-starkware]

Reviewed 3 of 4 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[avi-starkware]

crates/blockifier/src/concurrency/scheduler.rs line 105 at r5 (raw file):

        }

        Task::NoTaskAvailable

This case will usually mean that the status of the transaction wasn't right for Execution/Validation.

Suggestion:

Task::AskForTask

[avi-starkware]

crates/blockifier/src/concurrency/scheduler_test.rs line 61 at r5 (raw file):

    0,
    TransactionStatus::ReadyToExecute,
    Task::NoTaskAvailable

Suggestion:

    Task::AskForTask

[Yoni-Starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

crates/blockifier/src/concurrency/scheduler.rs line 105 at r5 (raw file):

Previously, avi-starkware wrote…

This case will usually mean that the status of the transaction wasn't right for Execution/Validation.

This means that there is no task available, and we should wait, right?

[avi-starkware]

crates/blockifier/src/concurrency/scheduler.rs line 105 at r5 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

This means that there is no task available, and we should wait, right?

When we reach this line, it is usually because the transaction's status at the execution_index is not ReadyToExecute.

[Yoni-Starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

crates/blockifier/src/concurrency/scheduler.rs line 105 at r5 (raw file):

Previously, avi-starkware wrote…

When we reach this line, it is usually because the transaction's status at the execution_index is not ReadyToExecute.

Done.

[Yoni-Starkware]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

crates/blockifier/src/concurrency/scheduler.rs line 95 at r6 (raw file):

        let index_to_execute = self.execution_index.load(Ordering::Acquire);

        if min(index_to_validate, index_to_execute) >= self.chunk_size {

@barak-b-starkware FYI: I restored this check in order to know when to stop the busy loop

[avi-starkware]

Reviewed 5 of 5 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1)