Improve benchmark runner to support stateless setup / measure / teardown

[cburgdorf]

What is wrong?

Currently when multiple benchmarks need to share some setup, they have to pass state around. We could probably improve the runner to handle that more elegantly in a stateless fashion.

How can it be fixed

Citing from here

One option (which would require changes to the core benchmark runner) would be something like:

def transfer_benchmark(w3, chain):
  # setup
  undeployed_contract = make_contract_for_deployment(w3)
  token_contract = erc20_deploy(w3, chain, undeployed_contract)
  txn_data = w3...buildTransaction().data
  yield

  # benchmark
  erc20_transfer(chain, txn_data, token_contract.address)
  yield

  # cleanup
  # (but no cleanup to do on this one, I think)
  yield

  # never hit this point

The test runner would roughly do:

def benchmark_run(benchmark: Callable):
  test_lifecycle = executor(w3, chain)
  next(test_lifecycle)  # setup

  start_clock ...
  next(test_lifecycle)  # run
  end_clock ...

  next(test_lifecycle)  # cleanup

[Bhargavasomu]

@cburgdorf I would like to work on this issue. Could you explain about the issue in a bit more detail.

[cburgdorf]

Hey @Bhargavasomu happy to hear you are interested to work on this. That said, I noticed you dropped a similar line on a bunch of other issues and my suggestion would be to stick to one issue at a time and just try to work your way through it before jumping on another one.

This particular issue was explained further in this comment but it most likely goes hand in hand with a bit of refactoring of existing benchmarks as well as the runner itself.

[Bhargavasomu]

Yaa will follow your advice @cburgdorf. Please don't mind, I'm just enthusiastic in working on these issues.

Regarding this particular issue, could you give me one example where the setup is being shared. I got this doubt because I looked into scripts/run.py to get the list of benchmarks and TO_EXISTING_ADDRESS_CONFIG, TO_NON_EXISTING_ADDRESS_CONFIG are the only configurations that are being shared. Is there anything I am missing?

[cburgdorf]

Please don't mind, I'm just enthusiastic in working on these issues.

Happy to hear!

Regarding this particular issue, could you give me one example where the setup is being shared

I think if we follow this refactoring, we could basically drop the existing pattern of using a setup_benchmark method as seen here:

https://github.com/ethereum/py-evm/blob/3061a48d112b2ae7d94adec89000372dd4bcac2c/scripts/benchmark/checks/deploy_dos.py#L72-L77

https://github.com/ethereum/py-evm/blob/3061a48d112b2ae7d94adec89000372dd4bcac2c/scripts/benchmark/checks/erc20_interact.py#L76-L81

The _setup_benchmark method is then called as part of execute

https://github.com/ethereum/py-evm/blob/3061a48d112b2ae7d94adec89000372dd4bcac2c/scripts/benchmark/checks/erc20_interact.py#L89-L110

This issue isn't really well spec'ed out yet and more an impromptu brain dump for an idea that needs further exploration. Giving this a bit more thought I see issues with this approach that probably need some thought. E.g. if the runner does roughly this:

def benchmark_run(benchmark: Callable):
  test_lifecycle = executor(w3, chain)
  next(test_lifecycle)  # setup

  start_clock ...
  next(test_lifecycle)  # run
  end_clock ...

  next(test_lifecycle)  # cleanup

How does that work for benchmarks that do not need a setup phase. Requiring them to add a yield before the actual work just because the benchmark runner expects a setup phase sounds like some unnecessary friction to me.

I realize that my comment is probably creating more questions than answers but that may just be another hint that this really is more a rough idea for someone to explore to improve the API. To be honest, I believe there are more impactful things to work on when it comes to improving the benchmark suite. Two things come to my mind and I'll create issues for them in just a moment:

• run at least some benchmarks against the actual LevelDB (https://github.com/ethereum/py-evm/issues/1359)
• refactor benchmarks to use new chain builder tools (https://github.com/ethereum/py-evm/issues/1360)

[Bhargavasomu]

The benchmark state information is passed onto to the parent super class, and there it is being executed. This is what you meant by transfer of state info, right @cburgdorf ? And I don't understand what difference is caused by replacing the _setup_benchmark function with transfer_benchamark