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

[WIP] Add Stack and Computation Benchmark #1707

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Bhargavasomu
Copy link
Contributor

What was wrong?

As part of the discussion here, a new Stack Benchmark is needed.

How was it fixed?

A TestStack.sol contract was written which would serve as the benchmark.

Cute Animal Picture

put a cute animal picture link inside the parentheses

@Bhargavasomu
Copy link
Contributor Author

@pipermerriam I still haven't made this a benchmark as of yet, but this is my first time where I am running a contract on the EVM, and I just had a small doubt.

  • For the above TestStack Contract which you mentioned, I ran that code through our EVM, but the final stack values that I am seeing is [2729995958]. As per the above code shouldn't the expected value be [10,000].
  • If at all this is stored in Memory (which is in bytes), how could I obtain the final value of v, because I would like to put an assert statement there.

Or is there anything else I am missing? Could you please clarify? Thankyou

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

Can you rename scripts/stack_benchmark/contract_data to scripts/stack_benchmark/fixtures to better fit with our directory naming conventions elsewhere.

Can you work with @njgheorghita to package up the contract assets as an EthPM package? This would be good dogfooding of our own APIs and should allow us to leverage some higher level APIs for deploying and interacting with the contract and reduce some of the loading and parsing overhead in the test case.

@Bhargavasomu
Copy link
Contributor Author

Also you can find the above stated values in py36-stack-benchmark

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

I just realized I was commenting on a WIP so please just ignore anything I commented on that isn't done.

One change I'd ask for if you aren't already planning to is separating the renaming, added docstring, and other unrelated changes to a stand-alone PR -> eth1_hash,


block, receipt, computation = chain.apply_transaction(tx)
# print(computation._memory._bytes)
print(computation._stack.values)
Copy link
Member

Choose a reason for hiding this comment

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

We tend towards using logging instead of print statements for this type of thing. That way we don't have to comment/uncomment them and can control verbosity and output destination using the logging APIs.

tests/core/merkle-utils/test_merkle_trees.py Outdated Show resolved Hide resolved
@@ -2,6 +2,7 @@
envlist=
py{35,36}-{core,database,transactions,vm}
py{36}-{benchmark,beacon}
py{36}-{stack-benchmark}
Copy link
Member

Choose a reason for hiding this comment

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

Hrm, I'd favor this just being included in the main benchmark run instead of having a separate dedicated CI run.

@Bhargavasomu
Copy link
Contributor Author

Bhargavasomu commented Jan 9, 2019

@pipermerriam I am extremely sorry, I think I messed up with git and it is showing some of the previous commits as mine. Will fix it immediately. The Merkle Tree related things aren't supposed to be part of this PR.

@pipermerriam
Copy link
Member

For the benchmarking of this I think the final value doesn't matter very much. It's more about determining how many times the stack is pushed/popped from (and maybe even in what quantities).

I only just now realized that #1515 still hasn't been completed as it would provide an ideal API for this. Without that I think you're limited to manually measuring it using something like unittest.mock and then just hard coding the numbers in the benchmark.

Then, suppose the contract code results in 150 stack pushes and 150 stack pops, then you can output some metrics like N pops / second. You should probably run the computation 100 times or something like that to get a sufficiently large sample size.

Then later when #1515 is merged we can adjust the code to dynamically measure the number of push/pops.

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

2 participants