-
Notifications
You must be signed in to change notification settings - Fork 12
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
refactor: setup for standardizing benchmarks #2
Conversation
b52fc1d
to
8fb0bd6
Compare
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.
Nice!
.env.example
Outdated
export GAS_LIMIT=30000000 | ||
|
||
# skip type-checking to avoid its perfomance impact | ||
export TS_NODE_TRANSPILE_ONLY=1 |
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.
Should have newline at end of file. (I cat
files like this!)
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.
Done in f7f3b78
scripts/benchmark-ganache.sh
Outdated
@@ -0,0 +1,4 @@ | |||
. ./.env | |||
|
|||
# Uses Ganache v6 (@beta) |
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.
# Uses Ganache v6 (@beta) | |
# Uses Ganache v7 (@beta) |
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.
Fixed in f7f3b78!
. ./.env | ||
|
||
# Uses Ganache v6 (@beta) | ||
time yarn ts-node ./scripts/convex.ganache.ts |
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 assume we're okay with just running time
to measure the whole command?
I was thinking it might be neat to benchmark startup time separately from simulation time, but it'd require that each script output its own metrics broken out. Probably more trouble than it's worth, though.
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.
Agreed, I'd like to benchmark that as well. I wasn't sure if each tool exposed that easily so figured it's worth looking at in a future PR, with time
ing the whole command being a good start for now
clean :; dapp clean | ||
test :; dapp test --rpc-url ${ETH_RPC_URL} | ||
deploy :; dapp create Convex | ||
benchmark-all :; bash scripts/benchmark-all.sh |
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.
Aside: does this repo need to do any results verification? One or more tool's simulation could be totally inaccurate and we wouldn't know!
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.
Yep true! I was thinking of also reporting the gasUsed
of each tool in a future PR, but open to other ideas here also. Not sure if verifying state changes or comparing transaction traces is excessive for a benchmarking repo 🤔
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 was just thinking... if we're forking at the same block for each tool, just count the logs and make sure all tools report the same number of logs. Seems quick and dirty? Could also do a deep-equals on the JSON, but that's perhaps more prone to minute differences.
Anyway, this also might be something to manually test once and then not worry about until/unless this repo becomes used more seriously.
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.
True, just verifying once might be sufficient for now! Either way, now tracking all potential metrics in #3
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.
🚀
TODO