Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: reduce Result checks in Gasometer #11

Closed
wants to merge 1 commit into from

Conversation

mooori
Copy link

@mooori mooori commented Jul 8, 2022

Gasometer has a field inner: Result<Inner<'config>, ExitError>. That
field is often accessed in Gasometer's method record_dynamic_cost, which
involves a lot of ? operators. This was identified as inefficiency in #448 (2nd item).

To improve performance, a mutable reference to Inner is stored in a variable
and used througout record_dynamic_cost, which allows to remove most ? in
that method.

Snapshot::new() is added to avoid ownership issues when compiling with
--features tracing. Gasometer::snapshot() borrows self, which can be
avoided by using Snapshot::new instead.

NEAR gas reductions

Including this in aurora-engine makes the following tests fail due to NEAR gas
usage which is lower than the expected numbers:

tests::one_inch::test_1inch_liquidity_protocol
tests::repro::repro_5bEgfRQ
tests::repro::repro_8ru7VEA
tests::repro::repro_D98vwmi
tests::repro::repro_FRcorNv
tests::repro::repro_GdASJ3KESs
tests::uniswap::test_uniswap_exact_output
tests::uniswap::test_uniswap_input_multihop

Reproduction

tree
.
├── aurora-engine # @ develop branch
├── sputnikvm     # containing this PR's modifications

Patch aurora-engine to include the locally modified sputnikvm:

diff --git a/Cargo.toml b/Cargo.toml
index f27de56..1f4bedf 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -27,6 +27,12 @@ rpath = false
 lto = true
 opt-level = 3
 
+[patch.'https://github.com/aurora-is-near/sputnikvm.git']
+evm = { path = "../sputnikvm" }
+evm-core = { path = "../sputnikvm/core" }
+evm-gasometer = { path = "../sputnikvm/gasometer" }
+evm-runtime = { path = "../sputnikvm/runtime" }
+
 [workspace]
 resolver = "2"
 members = [

Run tests with:

cd aurora-engine
make check

@mooori mooori marked this pull request as ready for review July 8, 2022 10:04
@mfornet mfornet requested review from birchmd and joshuajbouw July 11, 2022 09:27
Copy link

@mfornet mfornet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@joshuajbouw what are the possibilities of adding this generic changes upstream?

@mooori
Copy link
Author

mooori commented Jul 19, 2022

The NEAR gas reduction numbers have been missing - posting them here:

---- tests::repro::repro_8ru7VEA stdout ----
thread 'tests::repro::repro_8ru7VEA' panicked at 'assertion failed: `(left == right)`                                 
  left: `242`,                                             
 right: `236`', engine-tests/src/tests/repro.rs:146:5                                                                 
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
                                                                                                                      
---- tests::repro::repro_5bEgfRQ stdout ----
thread 'tests::repro::repro_5bEgfRQ' panicked at 'assertion failed: `(left == right)`                                 
  left: `706`,                                             
 right: `687`', engine-tests/src/tests/repro.rs:146:5
                                                           
---- tests::repro::repro_D98vwmi stdout ----
thread 'tests::repro::repro_D98vwmi' panicked at 'assertion failed: `(left == right)`                                 
  left: `199`,                                             
 right: `194`', engine-tests/src/tests/repro.rs:146:5
                                                           
---- tests::repro::repro_GdASJ3KESs stdout ----                                                                       
thread 'tests::repro::repro_GdASJ3KESs' panicked at 'assertion failed: `(left == right)`                              
  left: `133`,               
 right: `130`', engine-tests/src/tests/repro.rs:146:5                                                                 

---- tests::repro::repro_FRcorNv stdout ----       
thread 'tests::repro::repro_FRcorNv' panicked at 'assertion failed: `(left == right)`
  left: `197`,                                             
 right: `192`', engine-tests/src/tests/repro.rs:146:5   
                                                                                                                      
---- tests::uniswap::test_uniswap_input_multihop stdout ----   
thread 'tests::uniswap::test_uniswap_input_multihop' panicked at 'assertion failed: `(left == right)`                 
  left: `123`,                                                                                                        
 right: `119`', engine-tests/src/tests/uniswap.rs:41:5                                                                
                                                                                                                      
---- tests::one_inch::test_1inch_liquidity_protocol stdout ----       
thread 'tests::one_inch::test_1inch_liquidity_protocol' panicked at '20 Tgas is not equal to 21 Tgas', engine-tests/src/test_utils/mod.rs:842:5                                                                                             

@birchmd
Copy link
Member

birchmd commented Nov 29, 2022

Merged in 148c6f9 (did not go into master since we have a hard time getting our changes into upstream; but it is included in our internal release https://github.com/aurora-is-near/sputnikvm/releases/tag/v0.37.2-aurora )

@birchmd birchmd closed this Nov 29, 2022
aleksuss added a commit to aleksuss/sputnikvm that referenced this pull request Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants