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 :white_check_mark:

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

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: 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]

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.

[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...

[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

[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.