From 0f90ee61fd9cf0c86248bf2862fdbe1fa963b22d Mon Sep 17 00:00:00 2001 From: Eagle941 <8973725+Eagle941@users.noreply.github.com> Date: Wed, 18 Sep 2024 23:02:07 +0100 Subject: [PATCH] chore: suggestions from code review Co-authored-by: Ara Adkins --- docs/BENCHMARK.md | 61 ++++++++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 32 deletions(-) diff --git a/docs/BENCHMARK.md b/docs/BENCHMARK.md index 7fd20445f0..269890a8c0 100644 --- a/docs/BENCHMARK.md +++ b/docs/BENCHMARK.md @@ -1,12 +1,10 @@ -# Addition of `VisitedPcs` trait +# Adding the `VisitedPcs` Trait -Current blockifier doesn't store the complete vector of visited program counters -for each entry point call in an invoke transaction. Instead, visited program -counters are pushed in a `HashSet`. This is a limiting factor to perform -profiling operations which require record of the full trace returned by -`cairo-vm`. More flexibility is added to the blockifier with the introduction of -trait `VisitedPcs` which allows the user to process visited program counters in -the most suitable way for the task. +The state of the blockifier as of commit XXX doesn't store the complete vector of visited program counters +for each entry-point in an invoke transaction. Instead, visited program +counters are pushed into a `HashSet`. Unfortunately this limits the ability to perform profiling operations, as many require a record of the full trace returned from the `cairo-vm`. + +In order to enable more in-depth tracing use-cases, we have introduced the `VisitedPcs` trait which allows the user to process the visited program counters as they see fit. ## Existing code @@ -27,7 +25,7 @@ pub struct CachedState { ## New code -`VisitedPcs` is an additional generic parameter of `CachedState`. +`VisitedPcs` is added as an additional generic parameter of `CachedState`. ```rust #[derive(Debug)] @@ -43,45 +41,45 @@ pub struct CachedState { ``` An implementation of the trait `VisitedPcs` is included in the blockifier with -the name `VisitedPcsSet` and it mimics the existing `HashSet`. Also, for +the name `VisitedPcsSet`. This mimics the existing `HashSet` usage of this field. For test purposes, `CachedState` is instantiated using `VisitedPcsSet`. -## Performance considerations +## Performance Considerations -Given the importance of the blockifier in the Starknet ecosystem, we want to -measure the performance impact of adding the trait `VisitedPcs`. The existing -bechmark `transfers` doesn't cover operations with `CachedState` therefore we -need to design new ones. We have created two new benchmarks: +Given the importance of the blockifier's performance in the Starknet ecosystem, measuring the impact of adding the aforementioned `VisitedPcs` trait is very important. The existing +bechmark `transfers` doesn't cover operations that use the `CachedState`, and therefore we +have designed new ones as follows: - `cached_state`: this benchmark tests the performance impact of populating `visited_pcs` (implemented using `VisitedPcsSet`) with a realistic amount of visited program counters. The size of the sets is taken from transaction - `0x0177C9365875CAA840EA8F03F97B0E3A8EE8851A8B952BF157B5DBD4FECCB060` in the - mainnet. This transaction has been chosen randomly, but there is no assurance + `0x0177C9365875CAA840EA8F03F97B0E3A8EE8851A8B952BF157B5DBD4FECCB060` on + mainnet. This transaction has been chosen randomly but there is no assurance that it's representative of the most common Starknet invoke transaction. This benchmark tests the write performance of visited program counters in the state struct. - `execution`: this benchmark simulates a whole invoke transaction using a dummy contract. -## Performance impact +## Performance Impact -A script `bench.sh` has been added to benchmark the performance impact of these -changes: it is called as -`bash scripts/bench.sh 14e6a87722c1d0c757b1aa2756ffabe3f248fd7d e39ae0be4cec31938399199e0a1070279b4a78ed`. -The computer running the benchmark is: Debian VM over Windows 10 with VMWare -Workstation 17, i9-9900K, 64GB RAM, Samsung 990 Pro NVME SSD. +The `bench.sh` script has been added to benchmark the performance impact of these +changes. -The Rust toolchain used is: -``` -1.78-x86_64-unknown-linux-gnu (default) -rustc 1.78.0 (9b00956e5 2024-04-29) -``` +The benchmark results presented below were conducted under the following conditions: + +- **Operating System:** Debian (WHICH VERSION) running in a VMWare Workstation 17 VM on Windows 10 (WHICH VERSION) +- **Hardware:** i9-9900K @ XXX GHz, 64GB of RAM, Samsung 990 Pro NVMe SSD. +- **Rust Toolchain:** 1.78-x86_64-unknown-linux-gnu / rust 1.78.0 (9b00956e5 2024-04-29). + +The script was called as follows, but you may need to adjust the commit hashes in question to reproduce these results: + +`bash scripts/bench.sh 14e6a87722c1d0c757b1aa2756ffabe3f248fd7d e39ae0be4cec31938399199e0a1070279b4a78ed` -Noise threshold and confidence intervals are kept as per default Criterion.rs +The noise threshold and confidence intervals are kept as per default Criterion.rs configuration. -The results are shown in the following table: +The results are as follows: | Benchmark | Time (ms) | Time change (%) | Criterion.rs report | | ------------ | --------- | --------------- | ----------------------------- | @@ -89,5 +87,4 @@ The results are shown in the following table: | execution | 1.2882 | -1.7216 | Change within noise threshold | | cached_state | 5.2330 | -0.8703 | No change in performance | -The analysis of Criterion.rs determines that there isn't statistically -significant performance decrese. +Criterion's inbuilt confidence analysis suggests that these results have no statistical significant and do not represent real-world performance changes.