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

benchmarks: restore benchmarks #182

Merged
merged 3 commits into from
Jul 6, 2024

Conversation

apoelstra
Copy link
Member

In #128 we dropped the benchmarks. They can be restored with only minor tweaks, so do so.

@apoelstra
Copy link
Member Author

CI failure is just because nightly rustc has introduced a new Freeze trait which appears in the check-API check. There is a commit to fix this in #180 which I won't copy into here to avoid rebase trouble.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 82a577d

@apoelstra
Copy link
Member Author

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?

@tcharding
Copy link
Member

Needs api files updating @apoelstra

@tcharding
Copy link
Member

Also I've emailed Clark to see what is the status here and on #180.

Copy link
Member

@clarkmoody clarkmoody left a 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).
@apoelstra
Copy link
Member Author

Rebased, added a one-line warning fix from my error-correction-2 branch, and also added a benchmark for parameter generation.

Copy link
Member

@clarkmoody clarkmoody left a comment

Choose a reason for hiding this comment

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

ACK e0b95f1

@apoelstra apoelstra merged commit 86f7dc0 into rust-bitcoin:master Jul 6, 2024
13 checks passed
@apoelstra apoelstra deleted the 2024-03--restore-benches branch July 6, 2024 14:26
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.

3 participants