refactor: commit classes trie concurrently

[nimrod-starkware]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 72.00%. Comparing base (1d383c5) to head (e103dbe).
Report is 6 commits behind head on main.

Files	Patch %	Lines
...ter/src/patricia_merkle_tree/filled_tree/forest.rs	0.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #315      +/-   ##
==========================================
+ Coverage   70.59%   72.00%   +1.40%     
==========================================
  Files          38       38              
  Lines        2095     2211     +116     
  Branches     2095     2211     +116     
==========================================
+ Hits         1479     1592     +113     
- Misses        546      547       +1     
- Partials       70       72       +2     

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

[dorimedini-starkware]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/filled_tree/forest.rs line 61 at r1 (raw file):

            Arc::new(updated_forest.classes_trie),
            Arc::new(classes_updates),
        );

does the task start running the moment it's created?
in python, task t will not start running until someone awaits it;
perhaps you need to add this task to the JoinSet below?
And maybe better to create a top-level JoinSet with two tasks: one for the contracts tree and one for the classes tree (contracts tree JoinSet will have many storage tree tasks - i.e. it opens an inner JoinSet).
WDYT?

[nimrod-starkware]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/filled_tree/forest.rs line 61 at r1 (raw file):

Previously, dorimedini-starkware wrote…

does the task start running the moment it's created?
in python, task t will not start running until someone awaits it;
perhaps you need to add this task to the JoinSet below?
And maybe better to create a top-level JoinSet with two tasks: one for the contracts tree and one for the classes tree (contracts tree JoinSet will have many storage tree tasks - i.e. it opens an inner JoinSet).
WDYT?

good catch!
it didn't start running; now it starts immediately when using tokio::spawn.
is there any benefit to having two joinsets?

[github-actions]

Benchmark movements:
full_committer_flow performance improved 😺
full_committer_flow time: [28.898 ms 28.941 ms 28.986 ms]
change: [-4.1568% -2.8327% -1.7763%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
6 (6.00%) high mild

[dorimedini-starkware]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/filled_tree/forest.rs line 61 at r1 (raw file):

Previously, nimrod-starkware wrote…

good catch!
it didn't start running; now it starts immediately when using tokio::spawn.
is there any benefit to having two joinsets?

one inside the other, I think:

await JoinSet(class_trie_commitment_task, contract_trie_commitment_task)

where contract_tree_commitment_task is:

1. opening a JoinSet with all storage tries
2. awaiting them all
3. computing the contract trie

however, if tokio::spawn already starts the task then this current way is fine.
I'm only concerned about work stealing: if tokio::spawn moves context from current function to ClassesTrie::create, then you will start working on the classes trie before the other task(s) are spawned.

Maybe better to create both class_trie_commitment_task and contract_trie_commitment_task without them actually running, and then open a JoinSet for the two of them?

[nimrod-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/filled_tree/forest.rs line 61 at r1 (raw file):

Previously, dorimedini-starkware wrote…

one inside the other, I think:

await JoinSet(class_trie_commitment_task, contract_trie_commitment_task)

where contract_tree_commitment_task is:

1. opening a JoinSet with all storage tries
2. awaiting them all
3. computing the contract trie

however, if tokio::spawn already starts the task then this current way is fine.
I'm only concerned about work stealing: if tokio::spawn moves context from current function to ClassesTrie::create, then you will start working on the classes trie before the other task(s) are spawned.

Maybe better to create both class_trie_commitment_task and contract_trie_commitment_task without them actually running, and then open a JoinSet for the two of them?

is that better now?

[dorimedini-starkware]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware and @TzahiTaub)

---

crates/committer/src/patricia_merkle_tree/filled_tree/forest.rs line 61 at r1 (raw file):

Previously, nimrod-starkware wrote…

is that better now?

see below, maybe it's clearer

---

crates/committer/src/patricia_merkle_tree/filled_tree/forest.rs line 102 at r3 (raw file):

            Arc::new(contracts_trie_modifications),
        );
        let (classes_trie, contracts_trie) = join!(classes_trie_task, contracts_trie_task);

1. IIRC join! doesn't work as we would expect here, please confer with @aner-starkware
2. Now, as I understand it, you (a) compute storage tries, and (b) in parallel, compute the classes and the contracts tries. (a) and (b) are sequential. However, the classes trie computation is orthogonal to the storage/contract trie computations... execution tree should look something like this:

await join(
   task { compute(classes_trie) },
   task {
      await join(
         task { compute(storage_trie) } for storage_trie in storage_tries
      );
      task { compute(contract_trie) }
   }
)

it currently looks like this:

await join(
   task { compute(storage_trie) } for storage_trie in storage_tries
);
await join(
   task { compute(contract_trie) },
   task { compute(classes_trie) },
)

Code quote:

        let classes_trie_task = ClassesTrie::create::<TH>(
            Arc::new(updated_forest.classes_trie),
            Arc::new(classes_updates),
        );
        let contracts_trie_task = ContractsTrie::create::<TH>(
            Arc::new(updated_forest.contracts_trie),
            Arc::new(contracts_trie_modifications),
        );
        let (classes_trie, contracts_trie) = join!(classes_trie_task, contracts_trie_task);

[TzahiTaub]

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aner-starkware and @nimrod-starkware)

---

crates/committer/src/patricia_merkle_tree/filled_tree/forest.rs line 65 at r2 (raw file):

        let mut contracts_trie_modifications = HashMap::new();
        let mut filled_storage_tries = HashMap::new();
        let mut tasks = JoinSet::new();

Suggestion:

contracts_state_tasks

---

crates/committer/src/patricia_merkle_tree/filled_tree/forest.rs line 102 at r3 (raw file):

Previously, dorimedini-starkware wrote…

1. IIRC join! doesn't work as we would expect here, please confer with @aner-starkware
2. Now, as I understand it, you (a) compute storage tries, and (b) in parallel, compute the classes and the contracts tries. (a) and (b) are sequential. However, the classes trie computation is orthogonal to the storage/contract trie computations... execution tree should look something like this:

await join(
   task { compute(classes_trie) },
   task {
      await join(
         task { compute(storage_trie) } for storage_trie in storage_tries
      );
      task { compute(contract_trie) }
   }
)

it currently looks like this:

await join(
   task { compute(storage_trie) } for storage_trie in storage_tries
);
await join(
   task { compute(contract_trie) },
   task { compute(classes_trie) },
)

Does the join call order matter to anything? I would assume only the tasks creation order (the spawn call) is important.

[dorimedini-starkware]

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

---

crates/committer/src/patricia_merkle_tree/filled_tree/forest.rs line 102 at r3 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Does the join call order matter to anything? I would assume only the tasks creation order (the spawn call) is important.

the tasks.join_next().await above matters, it blocks (so class trie and contract trie tasks are only spawned after all storage tries are complete)

[TzahiTaub]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aner-starkware and @nimrod-starkware)

---

crates/committer/src/patricia_merkle_tree/filled_tree/forest.rs line 97 at r3 (raw file):

            Arc::new(updated_forest.classes_trie),
            Arc::new(classes_updates),
        );

This seems worst to me unless I'm missing something. Now we don't start computing the classes tri until after the storage tries computation finishes.

Code quote:

        let classes_trie_task = ClassesTrie::create::<TH>(
            Arc::new(updated_forest.classes_trie),
            Arc::new(classes_updates),
        );

[nimrod-starkware]

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

---

crates/committer/src/patricia_merkle_tree/filled_tree/forest.rs line 61 at r1 (raw file):

I'm only concerned about work stealing: if tokio::spawn moves context from current function to ClassesTrie::create, then you will start working on the classes trie before the other task(s) are spawned.

The spawn creates another task (which I guess will run on another core if we have more than one). The main thread will spawn the tasks for the storage tries and I think it will run in parallel, no?

---

crates/committer/src/patricia_merkle_tree/filled_tree/forest.rs line 97 at r3 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

This seems worst to me unless I'm missing something. Now we don't start computing the classes tri until after the storage tries computation finishes.

I agree

---

crates/committer/src/patricia_merkle_tree/filled_tree/forest.rs line 102 at r3 (raw file):

Previously, dorimedini-starkware wrote…

the tasks.join_next().await above matters, it blocks (so class trie and contract trie tasks are only spawned after all storage tries are complete)

you are right; the join! runs both tasks concurrently but not in parallel.

How about now?

[nimrod-starkware]

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

---

crates/committer/src/patricia_merkle_tree/filled_tree/forest.rs line 61 at r1 (raw file):

Previously, nimrod-starkware wrote…

I'm only concerned about work stealing: if tokio::spawn moves context from current function to ClassesTrie::create, then you will start working on the classes trie before the other task(s) are spawned.

The spawn creates another task (which I guess will run on another core if we have more than one). The main thread will spawn the tasks for the storage tries and I think it will run in parallel, no?

ohh now i understand... the classes trie on it's own may use more than one core..

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [33.854 ms 33.939 ms 34.042 ms]
change: [-5.2566% -3.9506% -2.8014%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
3 (3.00%) high mild
2 (2.00%) high severe

[TzahiTaub]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware)

[dorimedini-starkware]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)

[TzahiTaub]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)