-
Notifications
You must be signed in to change notification settings - Fork 19
benches: fix divmod_128_64 distribution #320
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
benches: fix divmod_128_64 distribution #320
Conversation
f7adad0 to
0f6cf69
Compare
apoelstra
left a comment
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.
On 0f6cf69 successfully ran local tests
|
The new distribution is consistently slower than the uniform one. But all results are in a 40-60ns range; I see maybe a 10% slowdown rather than the 2x (or maybe 4x) that I would naively expect. But I added printfs to the C code to confirm that with the new distribution we are no longer hitting the "shortcut" case, while with the uniform distribution we are. So this is working. I think this is just a case where the marshalling cost is overwhelming the actual computation cost. |
jets-bench/src/input.rs
Outdated
|
|
||
| // To implement this, we sample two random 64-bit strings, with one of them | ||
| // having its high bit forced to 1. The higher of the two (which will always | ||
| // have its high bit 1) is ah, and the other one is b. |
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.
The higher of the two is b.
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.
0f6cf69 to
9dc53e8
Compare
jets-bench/src/input.rs
Outdated
| } | ||
| unreachable!("if we get here, two uniform 63-bit samples were exactly equal") | ||
| } else { | ||
| panic!("invalid distribution {} for BIP 340 signature", dist) |
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.
Bad message.
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.
9dc53e8 to
8cb7e03
Compare
|
cc @canndrew |
| } | ||
|
|
||
| fn distribution_name(&self, dist: usize) -> String { | ||
| if dist == 0 { |
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.
This doesn't seem to match the implementation of sample below.
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.
lol oops, I had crippled sample to help with printf debugging. Good find!
|
LGTM other than the implementation of |
8cb7e03 to
888115c
Compare
|
@canndrew can you re-review the change? |
|
ACK 888115c |
888115c benches: fix divmod128_64 distribution (Andrew Poelstra) Pull request description: The `div_mod_128_64` jet has preconditions and shortcuts its operation if these are not met. To benchmark correctly we need to sample according to those preconditions. ACKs for top commit: canndrew: ACK 888115c Tree-SHA512: a9ddc32def17427bf2f28b4d8d41246bac1b1c71e97e8e3615797bf2e97ed728c5163427bd099a71d57004febebe2c9ba8768e4fe62722077a1f9b55b6e69778
The
div_mod_128_64jet has preconditions and shortcuts its operation if these are not met. To benchmark correctly we need to sample according to those preconditions.