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

Fix Jacobi benchmarks and other benchmark improvements #797

Merged
merged 4 commits into from
Sep 10, 2020

Conversation

sipa
Copy link
Contributor

@sipa sipa commented Aug 11, 2020

A number of improvements to bench_internal:

  • Less confusing variable naming
  • Use random Z coordinates for the (initial) gej variables (instead of 1).
  • Vary the inputs to the Jacobi symbol benchmarks (they were constantly testing the same input, giving a non-representative result).
  • Add a benchmark for testing gej_to_ge_var.

@sipa sipa changed the title Benchmark improvements (Jacobi symbols, Z randomization, gej_to_ge_var) Fix Jacobi benchmarks and other benchmark improvements Aug 12, 2020
Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

ACK mod nit

src/bench_internal.c Show resolved Hide resolved
@elichai
Copy link
Contributor

elichai commented Aug 13, 2020

Isn't it more meaningful to test it with p as the prime of the jacobi symbol? (tested locally and the result are very similiar, but I think that for purposes like schnorr this is what we care about)

EDIT: I see that bench_group_jacobi_var actually encapsulates this already.

@elichai
Copy link
Contributor

elichai commented Aug 13, 2020

FYI this seem to be slightly slower than the actual random average.

before this PR I got: (locked on 2.4Ghz)

scalar_inverse: min 15.8us / avg 15.8us / max 16.1us                                                                   
scalar_inverse_var: min 3.13us / avg 3.16us / max 3.24us                                                               
field_inverse: min 7.39us / avg 7.42us / max 7.57us                                                                                                                                                                                           
field_inverse_var: min 3.10us / avg 3.11us / max 3.12us                                                                                                                                                                                       
group_jacobi_var: min 1.23us / avg 1.24us / max 1.26us                                                                 
num_jacobi: min 1.01us / avg 1.04us / max 1.20us                                        

with this PR:

scalar_inverse: min 15.8us / avg 15.8us / max 16.0us
scalar_inverse_var: min 3.12us / avg 3.21us / max 3.48us
field_inverse: min 7.41us / avg 7.44us / max 7.55us
field_inverse_var: min 3.11us / avg 3.12us / max 3.24us
group_jacobi_var: min 3.67us / avg 3.68us / max 3.70us
num_jacobi: min 3.54us / avg 3.55us / max 3.57us

with randomly chosen inputs for field_inverse_var and num_jacobi:

scalar_inverse: min 15.8us / avg 15.8us / max 15.9us
scalar_inverse_var: min 3.13us / avg 3.15us / max 3.21us
field_inverse: min 7.39us / avg 7.41us / max 7.51us
field_inverse_var: min 3.16us / avg 3.21us / max 3.24us
group_jacobi_var: min 3.67us / avg 3.80us / max 4.43us
num_jacobi: min 3.42us / avg 3.44us / max 3.48us

(code diff for this https://pastebin.com/raw/ewuiYxGc)

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

ACK 38a7bb3. I can't explain @elichai's results but they seem to be close enough and preallocating random elements could be a separate improvement (because it gets rid of the normalize, add, etc noise).

@gmaxwell
Copy link
Contributor

ACK. I had assumed the different results with the precomputed values were probably largely alignment/cache/branch predictor issues. There has been some academic work to eliminate these effects by basically making a bunch of different binaries with the memory layout all randomized and benchmarking all of them. ... but unfortunately, like a billion other computer science research things that have shipped modified versions of clang, I wasn't able to get their code to work with the effort I was willing to put in.

I've contemplated before making benchmarks use precomputed values but it's also really nice if the benchmarks can run on low memory systems with minimal modification. Costs like add/etc could be subtracted out.

@sipa
Copy link
Contributor Author

sipa commented Sep 9, 2020

With the prospect of not needing Jacobi symbols anymore for BIP340 (and it possibly being removable from the codebase), the third commit may end up not being very useful long term. It doesn't hurt though.

I think this is ready.

@real-or-random
Copy link
Contributor

@sipa I think my comment above is still unresolved (#797 (comment))

The _x and _y suffices are confusing; they don't actually correspond
to X and Y coordinates. Instead replace them with arrays.
Also increase the number of fe inputs.
Also make the num_jacobi benchmark use the scalar order as modulus,
instead of a random number.
@real-or-random
Copy link
Contributor

ACK cb5524a

1 similar comment
@jonasnick
Copy link
Contributor

ACK cb5524a

@jonasnick jonasnick merged commit f3733c5 into bitcoin-core:master Sep 10, 2020
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 28, 2020
Summary:
 * Rename bench_internal variables

The _x and _y suffices are confusing; they don't actually correspond
to X and Y coordinates. Instead replace them with arrays.

 * Randomize the Z coordinates in bench_internal

Also increase the number of fe inputs.

 * Make jacobi benchmarks vary inputs

Also make the num_jacobi benchmark use the scalar order as modulus,
instead of a random number.

 * Add benchmark for secp256k1_ge_set_gej_var

This is a backport of secp256k1 [[bitcoin-core/secp256k1#797 | PR797]]

Test Plan:
  ninja bench-secp256k1

Reviewers: #bitcoin_abc, jasonbcox

Reviewed By: #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D7612
deadalnix pushed a commit to Bitcoin-ABC/secp256k1 that referenced this pull request Sep 29, 2020
Summary:
 * Rename bench_internal variables

The _x and _y suffices are confusing; they don't actually correspond
to X and Y coordinates. Instead replace them with arrays.

 * Randomize the Z coordinates in bench_internal

Also increase the number of fe inputs.

 * Make jacobi benchmarks vary inputs

Also make the num_jacobi benchmark use the scalar order as modulus,
instead of a random number.

 * Add benchmark for secp256k1_ge_set_gej_var

This is a backport of secp256k1 [[bitcoin-core/secp256k1#797 | PR797]]

Test Plan:
  ninja bench-secp256k1

Reviewers: #bitcoin_abc, jasonbcox

Reviewed By: #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D7612
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.

None yet

5 participants