starkware-libs / blockifier

Blockifier is a Rust implementation for the transaction-executing component in the StarkNet sequencer, in charge of creating state diffs and blocks.
Apache License 2.0
171 stars 99 forks source link

refactor(concurrency): use visited_pcs in execute_chunk #1988

Closed OriStarkware closed 1 month ago

OriStarkware commented 1 month ago

This change is Reviewable

codecov-commenter commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

barak-b-starkware commented 1 month ago

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 commented 1 month ago

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