-
Notifications
You must be signed in to change notification settings - Fork 660
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
base: main
Are you sure you want to change the base?
Conversation
139f45b
to
bc5a385
Compare
@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
Or is there anything else I am missing? Could you please clarify? Thankyou |
There was a problem hiding this 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.
Also you can find the above stated values in |
There was a problem hiding this 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) |
There was a problem hiding this comment.
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.
@@ -2,6 +2,7 @@ | |||
envlist= | |||
py{35,36}-{core,database,transactions,vm} | |||
py{36}-{benchmark,beacon} | |||
py{36}-{stack-benchmark} |
There was a problem hiding this comment.
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.
@pipermerriam I am extremely sorry, I think I messed up with |
bc5a385
to
870484e
Compare
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 Then, suppose the contract code results in 150 stack pushes and 150 stack pops, then you can output some metrics like Then later when #1515 is merged we can adjust the code to dynamically measure the number of push/pops. |
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