-
Notifications
You must be signed in to change notification settings - Fork 90
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
Benchmark Block struct serialization code #2018
Conversation
fefa9de
to
040b516
Compare
Thanks for this PR! This will benchmark serialization of the largest block test vector we have, but not the messages we support yet, so we should create another issue and resolve that with this PR vs #1775. And yes we should run all our benchmarks as part of CI, some of our dependencies (ed25519-zebra, redjubjub, bellman) have benchmarks for the batch verification of those signatures/proofs that we may want to run ourselves, or, repurpose them on top of the async services that leverage those batching implementations, and run those instead. |
large_multi_transaction_block(), | ||
), | ||
( | ||
"large_single_transaction_block", |
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.
👍
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.
Let's fix the hard-coded block name, and then we're good to merge.
You can also move the lexical-core change to #2026 if you want, but I don't want to block on that.
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.
Once CI passes, let's auto-merge this PR.
wow, I thought this checkbox only for editing initial message. TIL 👍 |
Motivation
For resolving #1774 there should be a way to determine that improvement was introduced. Benchmark is a good way to see performance changes.
I do not think that it's required to benchmark all structs which implement
ZcashSerialize
.Block
uses a lot (all?) of structs internally, so onlyBlock
should be enough.Solution
Add a simple benchmark for the biggest block from
zebra-test
created with criterion.Review
Related Issues
Should close #2019.
Follow Up Work
Create tracking issue for adding benchmarks to CI once this PR is merged.