refactor(concurrency): use visited_pcs in execute_chunk

[OriStarkware]

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.70%. Comparing base (4505d16) to head (dcfd474).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1988      +/-   ##
==========================================
+ Coverage   78.38%   79.70%   +1.31%     
==========================================
  Files          62       62              
  Lines        8855     9209     +354     
  Branches     8855     9209     +354     
==========================================
+ Hits         6941     7340     +399     
+ Misses       1475     1452      -23     
+ Partials      439      417      -22     

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

[Yoni-Starkware]

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

---

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

        chunk: &[Transaction],
    ) -> Vec<TransactionExecutorResult<TransactionExecutionInfo>> {
        use std::collections::HashMap;

Move up (use #[cfg(feature = "concurrency")])

Code quote:

    use std::collections::HashMap;

---

crates/blockifier/src/blockifier/transaction_executor.rs line 258 at r1 (raw file):

            tx_execution_results
                .push(locked_execution_output.result.map_err(TransactionExecutorError::from));
            visited_pcs.extend(locked_execution_output.visited_pcs);

You might be overriding existing keys. You should extend values like this function:

blockifier/crates/blockifier/src/state/cached_state.rs

Line 76 in 952373d

self.add_visited_pcs(*class_hash, class_visited_pcs);

Please share this code - extract it out of the CachedState and call the function merge_visited_pcs

Code quote:

visited_pcs.extend(locked_execution_output.visited_pcs);

[OriStarkware]

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

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Move up (use #[cfg(feature = "concurrency")])

Done.

---

crates/blockifier/src/blockifier/transaction_executor.rs line 258 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

You might be overriding existing keys. You should extend values like this function:

blockifier/crates/blockifier/src/state/cached_state.rs

Line 76 in 952373d

self.add_visited_pcs(*class_hash, class_visited_pcs);

Please share this code - extract it out of the CachedState and call the function merge_visited_pcs

Done.

[Yoni-Starkware]

Reviewed 3 of 4 files at r2, all commit messages.
Reviewable status: 3 of 4 files reviewed, all discussions resolved (waiting on @barak-b-starkware and @noaov1)

---

crates/blockifier/src/blockifier/transaction_executor.rs line 258 at r1 (raw file):

Previously, OriStarkware wrote…

Done.

LGTM but please share this piece of code

for (class_hash, class_visited_pcs) in locked_execution_output.visited_pcs {
                visited_pcs.entry(class_hash).or_default().extend(class_visited_pcs);

[barak-b-starkware]

crates/blockifier/src/blockifier/transaction_executor.rs line 261 at r2 (raw file):

                visited_pcs.entry(class_hash).or_default().extend(class_visited_pcs);
            }
        }

I like the fold_while style better. This use case fits it 100%, but it's a matter of style.
Then, instead of visited_pcs use results_visited_pcs.1 and instead of tx_execution_results use results_visited_pcs.0.
Non-blocking

Suggestion:

        let results_visited_pcs = worker_executor
            .execution_outputs
            .iter()
            .fold_while((Vec::new(), HashMap::<ClassHash, HashSet<usize>>::new()), |mut results_visited_pcs, execution_output| {
                if results_visited_pcs.0.len() >= n_committed_txs {
                    Done(results_visited_pcs)
                } else {
                    let locked_execution_output = execution_output
                        .lock()
                        .expect("Failed to lock execution output.")
                        .take()
                        .expect("Output must be ready.");
                    results_visited_pcs.0.push(
                        locked_execution_output.result.map_err(TransactionExecutorError::from),
                    );
                    for (class_hash, class_visited_pcs) in locked_execution_output.visited_pcs {
                        results_visited_pcs.1.entry(class_hash).or_default().extend(class_visited_pcs);
                    }        
                    Continue(results_visited_pcs)
                }
            })
            .into_inner();

[barak-b-starkware]

AFAIU we use the visited_pcs in the finalize method of the TransactionExecutor.
I think we need tests for that, maybe some flow that involves execute_chunk and finalize. Maybe more unit tests?
BTW I think we don't have any test for finalize of the TransactionExecutor.

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1 and @OriStarkware)

[Yoni-Starkware]

Correct. The tx executor test is also relatively new. We should add unit tests, not necessarily in this PR.
Currently, it is tested by python's flow tests

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1 and @OriStarkware)

[barak-b-starkware]

crates/blockifier/src/blockifier/transaction_executor.rs line 261 at r2 (raw file):

Previously, barak-b-starkware wrote…

I like the fold_while style better. This use case fits it 100%, but it's a matter of style.
Then, instead of visited_pcs use results_visited_pcs.1 and instead of tx_execution_results use results_visited_pcs.0.
Non-blocking

Even better, let (tx_execution_results, visited_pcs) instead of let results_visited_pcs and then no need to use the results_visited_pcs.0/1

[OriStarkware]

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

---

crates/blockifier/src/blockifier/transaction_executor.rs line 258 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

LGTM but please share this piece of code

for (class_hash, class_visited_pcs) in locked_execution_output.visited_pcs {
                visited_pcs.entry(class_hash).or_default().extend(class_visited_pcs);

add_visited_pcs is part of the state trait, so code sharing will not be elegant.

---

crates/blockifier/src/blockifier/transaction_executor.rs line 261 at r2 (raw file):

Previously, barak-b-starkware wrote…

Even better, let (tx_execution_results, visited_pcs) instead of let results_visited_pcs and then no need to use the results_visited_pcs.0/1

I think that it is more readable with the for.

[noaov1]

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

Non blocking comments