-
Notifications
You must be signed in to change notification settings - Fork 50
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
benchmarks: restore benchmarks #182
benchmarks: restore benchmarks #182
Conversation
CI failure is just because nightly rustc has introduced a new |
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.
ACK 82a577d
cc @clarkmoody this is a quick/easy PR. Can you do a quick review? Alternately, would you be okay with my merging things here with only ACKs from @tcharding? |
Needs api files updating @apoelstra |
Also I've emailed Clark to see what is the status here and on #180. |
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.
ACK 82a577d
Found with `cargo +nightly clippy --all-targets`.
In rust-bitcoin#128 we dropped the benchmarks. They can be restore with only minor tweaks, so do so.
Mainly to confirm that these are really expensive (on my system, mulitple milliseconds for the descriptor checksum) and that we should precompute these values rather than suggesting users compute them at run-time whenever they want to do correction (which would result in a simpler API).
82a577d
to
e0b95f1
Compare
Rebased, added a one-line warning fix from my |
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.
ACK e0b95f1
In #128 we dropped the benchmarks. They can be restored with only minor tweaks, so do so.