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

Parallel EVM Tests #444

Merged
merged 27 commits into from Jan 13, 2022
Merged

Parallel EVM Tests #444

merged 27 commits into from Jan 13, 2022

Conversation

gakonst
Copy link
Member

@gakonst gakonst commented Jan 13, 2022

Co-authored with @brockelmore, converts unit & fuzz tests to be run in parallel.

This achieves a significant speedup, especially for fuzz tests.

Things in this PR:

  1. Instead of passing EVM into the runner, pass EvmOpts (refactor EvmOpts to evm-adapters)
  2. Instantiate an EVM on each test (via new_sputnik_evm, this tightly couples us to SputnikEVM a bit, but should be fine, we weren't using evmodin either way)
  3. Use the SharedBackend (instead of ForkMemoryBackend), which is cheap to clone, but allows sharing state across forked tests, deduplicating work as much as possible
  4. Instantiating an EVM per test lets us make our functions &self instead of &mut self, which makes parallelization with rayon very easy: we just replace .iter() with par_iter().

Supersedes #435
Also contains the fixes from #430.

  • Benchmark
    • @brockelmore tested spells-mainnet from 2m37 go to 1m43 (~30% improv)
    • guni-lev from to 33s to 27.7s (~20% improvement)
    • drai from ~18s to ~9s (2x improv)
    • solmate from 2m48 to 47s (3.5x)
  • Figure out the todo-item in evm.debug_calls() in the CLI's run.rs (fix debugging #445)

This allows us to instantiate as many EVMs as we want inside of the runner,
which in turn will enable running tests in parallel
previously we'd reuse the same EVM, now, we use a different EVM
per test, allowing us to get rid of the mutable reference on self
There's a TODO here around how we should do the evm.debug_calls check which we should figure out
Copy link
Member

@brockelmore brockelmore left a comment

Choose a reason for hiding this comment

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

LGTM gg

cli/src/cmd/run.rs Outdated Show resolved Hide resolved
cli/src/cmd/run.rs Show resolved Hide resolved
@sambacha
Copy link
Contributor

what are your thoughts for this item @gakonst

  • Test for edge cases with SharedBackend

@gakonst
Copy link
Member Author

gakonst commented Jan 13, 2022

Manual testing for now, not sure if we can properly integration test & benchmark yet.

Comment on lines +151 to +154
self.run_tests(name, abi, backend, deploy_code.clone(), filter)?
}
BackendKind::Shared(ref backend) => {
self.run_tests(name, abi, backend, deploy_code.clone(), filter)?
Copy link
Member

Choose a reason for hiding this comment

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

cloning is a bit expensive,
perhaps we can at a later stage either consume on fn test or remove the bytecode or wrap it in an arc

Copy link
Member Author

Choose a reason for hiding this comment

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

ethers::types::Bytes is a wrapper around bytes::Bytes, which is cheap to clone https://docs.rs/bytes/latest/bytes/struct.Bytes.html

Copy link
Member

Choose a reason for hiding this comment

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

ah right, great!

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm,
sputnik's borrowing behavior makes it a bit more verbose than it should be

@gakonst
Copy link
Member Author

gakonst commented Jan 13, 2022

@brockelmore said he'll take over the conflict & merge, else I'll wrap it in the morning

@gakonst gakonst merged commit 764c69c into master Jan 13, 2022
@gakonst gakonst deleted the parallel-tests branch January 13, 2022 21:43
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.

None yet

4 participants