Context isn't freed in the ECDH benchmark #722

elichai commented at 2:40 PM on March 4, 2020: contributor

Hi, Currently the context is leaked in bench_ecdh. Wanted to prevent this happening in the future by adding it to the valgrind run in travis. for that I made run_benchmark pass the number of iterations as a variable into the benchmarks so that the iteration number it uses to print results actually matches the one used in the benchmark. and added a way to pass -DITERS=N to change the baseline number of iterations used in the benchmarks(so that we can run the benchmarks with ITERS=1 under travis to make sure they compile, run, don't leak or have UB)

free the ctx at the end of bench_ecdh 02dd5f1bbb

elichai force-pushed on Mar 4, 2020

real-or-random commented at 3:21 PM on March 4, 2020: contributor

Concept ACK.

This is neat. Travis fails at the moment: https://travis-ci.org/bitcoin-core/secp256k1/jobs/658249366#L447

A few observations:

If we already make iter configurable, wouldn't it be more convenient to obtain the parameter at runtime?
Should we make count configurable too?

elichai commented at 3:27 PM on March 4, 2020: contributor

* If we already make `iter` configurable, wouldn't it be more convenient to obtain the parameter at runtime?

That's what I did at first, but it somewhat collides with have_flag so I decided to start with a simpler version and if people think it's a useful feature I can implement have_flag in a way that will work both with the number of iterations and the flags without ambiguity(ie --tests=N)

* Should we make `count` configurable too?

Count doesn't seem to affect performance that match, so I didn't need to do that. but if we'll add runtime configurations we can definitely add that too.

elichai commented at 3:31 PM on March 4, 2020: contributor

Concept ACK.

This is neat. Travis fails at the moment: https://travis-ci.org/bitcoin-core/secp256k1/jobs/658249366#L447

Right, forgot I need to check if recovery and/or ecdh are compiled or not. that will be a big enough logic that I'm thinking of moving this to autotools maybe (although that will make it hard again to run under valgrind) Will fix this later/tomorrow and see how complex it will look

elichai force-pushed on Mar 4, 2020

sipa commented at 3:16 AM on March 5, 2020: contributor

configure: error: unrecognized option: `-DITERS=1'

elichai force-pushed on Mar 5, 2020

jonasnick commented at 9:06 PM on March 9, 2020: contributor

I agree that it would be better to make the benchmark iterations runtime configurable. Perhaps getenv would be an acceptable workaround?

real-or-random commented at 7:37 AM on March 10, 2020: contributor

I agree that it would be better to make the benchmark iterations runtime configurable. Perhaps getenv would be an acceptable workaround?

I think an environment variable is not bad because it can persist and it applies to all benchmarks. That fits our usage of the benchmarks.

elichai force-pushed on Mar 11, 2020

in src/bench.h:125 in 98812c5d2f outdated

 120 | @@ -121,4 +121,13 @@ int have_flag(int argc, char** argv, char *flag) {
 121 |      return 0;
 122 |  }
 123 |  
 124 | +int get_iters(int default_iters) {
 125 | +    char* env = getenv("ITERS");

real-or-random commented at 11:00 AM on March 11, 2020:

maybe change it to something with more meaning and less likely to collide with other software, e.g., SECP256K1_BENCH_ITERS

elichai commented at 11:01 AM on March 11, 2020:

even though it's just the for the benchmarks? we also have int have_flag(..); and void print_number(..); in that header.

real-or-random commented at 11:06 AM on March 11, 2020:

I'm talking about the environment variable, not the function. :)

elichai commented at 11:13 AM on March 11, 2020:

Done

real-or-random approved

real-or-random commented at 11:00 AM on March 11, 2020: contributor

ACK except nit

elichai force-pushed on Mar 11, 2020

real-or-random approved

real-or-random commented at 11:50 AM on March 11, 2020: contributor

ACK https://github.com/bitcoin-core/secp256k1/pull/722/commits/31c3d6ccda432b35d1d83b72bcb9ba6f3c0a3166 I've looked at the diff but I haven't tested the changes

in .travis.yml:97 in 31c3d6ccda outdated

  93 | @@ -94,6 +94,12 @@ script:
  94 |     travis_wait 30 valgrind --error-exitcode=42 ./tests 16 &&
  95 |     travis_wait 30 valgrind --error-exitcode=42 ./exhaustive_tests;
  96 |     fi
  97 | + - if [ -n "$BENCH" ]; then

jonasnick commented at 8:26 PM on March 11, 2020:

BENCH is never set

elichai commented at 8:41 PM on March 11, 2020:

oh I broke it when I replaced the precompiled macro with the env. Thanks!

elichai commented at 9:49 AM on March 13, 2020:

Done

jonasnick commented at 6:45 PM on March 16, 2020:

Better to unset BENCH int the BUILD=distcheck case similar to CTIMETEST because the bench binaries are not build in that case.

elichai commented at 2:03 PM on March 18, 2020:

Hmm so why didn't the tests fail

elichai commented at 2:05 PM on March 18, 2020:

Maybe because there's ; between the ifs? hmmm

elichai commented at 2:16 PM on March 18, 2020:

Got it to fail: https://travis-ci.org/github/elichai/secp256k1/jobs/663963345 Will fix now. Thanks.

real-or-random commented at 2:18 PM on March 18, 2020:

ugh yep, I think we want && to make sure that the final exit code of the statement.

I think it's cleaner to have individual lines in yaml even.

real-or-random commented at 2:20 PM on March 18, 2020:

I think it's cleaner to have individual lines in yaml even.

Then the execution continues. With && we stop after the first failing command, which is okay but not optimal.

elichai commented at 10:43 AM on March 19, 2020:

Can you split an if across different yaml lines?

real-or-random commented at 12:33 PM on March 19, 2020:

Now, but you could different ifs with combined conditions on different lines. If you want to try, feel free to add a commit on top. Otherwise tell us and let's get his merged, it's certainly good as it is.

elichai commented at 12:47 PM on March 19, 2020:

I'm kinda tired of fighting with travis, so unless there's any objections I prefer to get it in as is

in src/bench_internal.c:375 in 3f5e420c27 outdated

 394 | +    if (have_flag(argc, argv, "hash") || have_flag(argc, argv, "sha256")) run_benchmark("hash_sha256", bench_sha256, bench_setup, NULL, &data, 10, iters);
 395 | +    if (have_flag(argc, argv, "hash") || have_flag(argc, argv, "hmac")) run_benchmark("hash_hmac_sha256", bench_hmac_sha256, bench_setup, NULL, &data, 10, iters);
 396 | +    if (have_flag(argc, argv, "hash") || have_flag(argc, argv, "rng6979")) run_benchmark("hash_rfc6979_hmac_sha256", bench_rfc6979_hmac_sha256, bench_setup, NULL, &data, 10, iters);
 397 |  
 398 |      if (have_flag(argc, argv, "context") || have_flag(argc, argv, "verify")) run_benchmark("context_verify", bench_context_verify, bench_setup, NULL, &data, 10, 20);
 399 |      if (have_flag(argc, argv, "context") || have_flag(argc, argv, "sign")) run_benchmark("context_sign", bench_context_sign, bench_setup, NULL, &data, 10, 200);

jonasnick commented at 8:29 PM on March 11, 2020:

Is this and context_verify intentionally left with 200 iters? Would be more consistent to use 1 + iters/100 or something.

elichai commented at 8:42 PM on March 11, 2020:

nope. missed it

elichai commented at 9:49 AM on March 13, 2020:

Done

in src/bench_ecmult.c:192 in 3f5e420c27 outdated

 191 |      }
 192 |  
 193 | -    for (p = 0; p <= 11; ++p) {
 194 | -        for (i = 9; i <= 16; ++i) {
 195 | -            run_test(&data, i << p, 1);
 196 | +    if (iters > 1) {

jonasnick commented at 8:30 PM on March 11, 2020:

Would be good to add comment why that's useful.

elichai commented at 9:49 AM on March 13, 2020:

Done

in src/bench_sign.c:17 in 3f5e420c27 outdated

  13 | @@ -14,6 +14,7 @@ typedef struct {
  14 |      unsigned char key[32];
  15 |  } bench_sign;
  16 |  
  17 | +

jonasnick commented at 8:31 PM on March 11, 2020:

nit: There are 2 more unnecessary newlines I think.

elichai commented at 9:49 AM on March 13, 2020:

Done

gmaxwell commented at 10:40 PM on March 11, 2020: contributor

FWIW, when you setup 'fast' CI tests, please at least run them with 2 iterations, not 1. 1 iteration will fail to catch some kinds of bugs in the test harnesses, e.g. use after free if the deallocations are in the wrong place with the testing loops, etc. If 2 is too much, maybe resize the underlying work units so that they're not.

Pass num of iters to benchmarks as variable, and define envvar ca4906b02e

elichai force-pushed on Mar 13, 2020

jonasnick commented at 6:45 PM on March 16, 2020: contributor

ACK mod nit

Add running benchmarks regularly and under valgrind in travis 85b35afa76

elichai force-pushed on Mar 18, 2020

jonasnick approved

jonasnick commented at 10:20 PM on March 18, 2020: contributor

ACK 85b35afa765ba23ac3682cf15800769b0a3b834d

jonasnick cross-referenced this on Mar 24, 2020 from issue Consider removing x86_64 field assembly by jonasnick

real-or-random commented at 2:35 PM on March 24, 2020: contributor

ACK 85b35afa765ba23ac3682cf15800769b0a3b834d I looked at the diff and tested the ecdh benchmark using valgrind

jonasnick merged this on Mar 24, 2020

jonasnick closed this on Mar 24, 2020

real-or-random commented at 4:08 PM on March 24, 2020: contributor

This is minor inconvenience that I can fix on the way, e.g. in #723:

> SECP256K1_BENCH_ITERS= ./bench_ecdh 
[1]    96654 floating point exception (core dumped)

deadalnix referenced this in commit 270f3194d8 on Jun 4, 2020

deadalnix referenced this in commit a29a90ca04 on Jun 5, 2020

sipa cross-referenced this on Jun 9, 2020 from issue Update libsecp256k1 subtree by sipa

fanquake referenced this in commit 8c97780db8 on Jun 13, 2020

sidhujag referenced this in commit 8a3a072968 on Jun 13, 2020

ComputerCraftr referenced this in commit b98f1c6e6c on Jun 16, 2020

UdjinM6 referenced this in commit 9d36ba6570 on Aug 10, 2021

5tefan referenced this in commit 8ded2caa74 on Aug 12, 2021

gades referenced this in commit d855cc511d on May 8, 2022