Restrict running Valgrind #282

minad · 2019-05-21T09:39:29Z

Activate on the develop branch
Activate on branches containing the word v-algrind (without hyphen)
Activate if the commit message contains v-algrind (without hyphen)

With valgrind the tests take about 15 minutes, without valgrind about 1 minute. We should not do that. I think it is bad for us and it is kind of abusive towards travis.

minad · 2019-05-21T09:46:36Z

@sjaeckel I added a --with-travis-valgrind option which has the travis specific behavior. Please do us a favor and merge this, update all PRs on top of develop and be done with all of it in two minutes ;)

minad · 2019-05-21T09:47:26Z

Ah, I see you already approved. But I think you might like --with-travis-valgrind better.

sjaeckel · 2019-05-21T09:50:16Z

I think you might like --with-travis-valgrind better

indeed I do

Please do us a favor and merge this, update all PRs on top of develop and be done with all of it in two minutes

that's why I already killed all other PR's running besides that run that is nearly finished ;-)
I'll merge then that one, rebase this and we'll see how long it will take :)

minad · 2019-05-21T10:12:17Z

@sjaeckel For some reason it is still running valgrind. Travis says "branch develop" but this is a PR. Let me check.

minad · 2019-05-21T10:13:14Z

Urks. TRAVIS_BRANCH: for builds triggered by a pull request this is the name of the branch targeted by the pull request.

Fix incoming...

minad · 2019-05-21T10:24:49Z

@sjaeckel Works now. We will see if it still valgrinds on develop after you merged it ;)

sjaeckel · 2019-05-21T10:27:56Z

We could probably add a single valgrind build as one of the first so we're sure that at least the default config is valgrinded before merging a PR!?

minad · 2019-05-21T10:30:12Z

I also thought about it and I think it is unnecessary. But if you want it no problem, only one is much more acceptable imho.

sjaeckel · 2019-05-21T10:33:10Z

I think it is unnecessary

I think it won't be needed very often, but I'd prefer to have for those few cases

only one is much more acceptable imho

I also think so! that's how we do it for libtomcrypt as well, only the default build gets valgrinded

minad · 2019-05-21T10:33:23Z

Ah I rejected the idea since we would need valgrind on MP_8BIT MP_16BIT etc to be safe. And then it is again open ended.

minad · 2019-05-21T10:35:54Z

the most restrictive build is with CONV_WARNINGS=strict but this one also enables enum which is not yet the default ABI

sjaeckel · 2019-05-21T10:36:45Z

we would need valgrind on MP_8BIT MP_16BIT etc to be safe

I don't think that's required!

especially since 8 and 16bit shouldn't be treated as "production use"
there's not that much different code for those configurations

.travis.yml

* Activate on the develop branch * Activate on branches containing the word v-algrind (without hyphen) * Activate if the commit message contains v-algrind (without hyphen) * Run default build always with valgrind

minad · 2019-05-21T11:40:23Z

@sjaeckel Could we run the tuner only for 64 bit and not for all the cases? It also needs quite some time. We should run it at least once to make sure that nothing breaks by accident. Otherwise the tuner is probably low churn and we don't have to test in in all configs.

The timer is also a library consumer and probably not affected by MP_8BIT etc. Well, the result is - but not the procedure by itself.

sjaeckel · 2019-05-21T11:42:05Z

Could we run the tuner only for 64 bit and not for all the cases?

perfectly fine by me!

sjaeckel · 2019-05-21T11:43:38Z

btw. I've just discovered https://travis-ci.org/libtom/libtommath/jobs/535171987 by hazard... should we have error'ed on this? I can't reproduce it

minad · 2019-05-21T11:44:49Z

You mean the malloc reachable memory? At first this is not really a bug since it is still reachable and it happens in the dynamic loader. I've seen such things quite often, ld.so and dlopen often leaks memory in some way but this has nothing to do with us.

EDIT: Ok has nothing todo with the dynamic loader, but with libstdc++. I don't understand where c++ is coming from here.

EDIT2: We should probably compile with debug data enable. Are we doing this already? Then valgrind might give us more infos if the call is coming from us. Enabling -g has no downsides, it just adds dwarf.

... as they take the longest time

sjaeckel · 2019-05-21T11:51:05Z

We should probably compile with debug data enable.

good idea for the valgrind build!

czurnieden · 2019-05-21T12:01:21Z

especially since 8 and 16bit shouldn't be treated as "production use"

It needs to treated as such as long as it is in LTM.

And the 16-bit version is quite usable with a lot of 16-bit MCUs, says Mouser.

The 8-bit MCUs on the other side have rarely enough memory available for even the most reduced version of LTM. Only some of the 8-bit "classicals" come close.

there's not that much different code for those configurations

There is no different code for MP_32BIT (only setting the types for mp_digit/mp_word)

There is no different code for MP_16BIT (only testing/setting the types) with the exception of bn_mp_montgomery_setup.c which is just some math and can be safely assumed as working after all these years of testing.

There is different code for MP_8BIT and some of it is in cryptographically relevant code, so it still needs testing of function and memory use.
Or get rid of the 8-bit branch completely.
I wouldn't shed a single tear if you would, it's a real pain in the behind.

And honestly: such large test-suites are not really useful for the average commit. One compiler with all hardening options switched on, (with and) without memory tests for 8(, 16, 32,) and 64 bit (and --format ;-) ) is sufficient for daily use.

Maybe another option like the one switching memory-tests on/off to switch the whole test for that environment on/off?

minad · 2019-05-21T12:10:21Z

@czurnieden

And honestly: such large test-suites are not really useful for the average commit. One compiler with all hardening options switched on, (with and) without memory tests for 8(, 16, 32,) and 64 bit (and --format ;-) ) is sufficient for daily use.

I think what we have here is a good compromise. Average PRs are valgrinded only once and develop is fully valgrinded. I like it like this!

sjaeckel · 2019-05-21T12:12:39Z

I like it like this!

I like it as well

and now that this single valgrind build takes longer than all the other builds together we can also leave all the others enabled :D

minad · 2019-05-21T12:14:02Z

and now that this single valgrind build takes longer than all the other builds together we can also leave all the others enabled :D

Except for the tuner. It is too slow, @czurnieden! :D

sjaeckel · 2019-05-21T12:15:46Z

Except for the tuner. It is too slow, @czurnieden! :D

I can live with the speed of the tuner :-) as long as it's faster than the valgrind build it's fine ;-)

minad · 2019-05-21T12:25:12Z

@sjaeckel COMPILE_DEBUG needlessly disables optimizations. This was written in former times when -O2/O3 was not compatible with -g. I have to change this. The valgrind build takes 30 minutes now 😠

If you additionally want to disable optimizations, define this IGNORE_SPEED macro.

[skip ci]

minad · 2019-05-21T12:53:55Z

@sjaeckel Now it works, but the sanitized clang build is probably not the best idea for valgrind - instead I activate valgrind on the gcc 4.9 build without sanitizer and run it first. Do you agree? The sanitized clang build takes around 25 minutes, the gcc 4.9 build 17 minutes.

If we want to optimize more we could also disable installing valgrind everytime etc. But this is probably not worth it. We got the long hanging fruits now.

[skip ci]

sjaeckel · 2019-05-21T12:56:55Z

We got the long hanging fruits now

🍌

minad · 2019-05-21T12:59:19Z

Now I am done. I won't touch this anymore. I promise.

so all long-running jobs start in the beginning [skip ci]

czurnieden · 2019-05-21T14:23:07Z

Except for the tuner. It is too slow, @czurnieden! :D

With the new #265

czurnieden ~/GITHUB/libtommath/etc  X (faster_toom_cook_three)$ time make tune
cc -Wall -W -Wextra -Wshadow -O3 -I../   -c -o tune.o tune.c
cc -Wall -W -Wextra -Wshadow -O3 -I../ tune.o ../libtommath.a -o tune
./tune_it.sh
You might like to watch the numbers go up to 100 but it will take a long time!
100Writing cut-off values to "../tommath_cutoffs.h".
In case of failure: a copy of "../tommath_cutoffs.h" is in "../tommath_cutoffs.h.orig"
'../tommath_cutoffs.h' -> '../tommath_cutoffs.h.orig'
#define MP_DEFAULT_KARATSUBA_MUL_CUTOFF 94
#define MP_DEFAULT_KARATSUBA_SQR_CUTOFF 127
#define MP_DEFAULT_TOOM_MUL_CUTOFF      165
#define MP_DEFAULT_TOOM_SQR_CUTOFF      216

real	0m13,760s
user	0m13,432s
sys	0m0,056s

Better? ;-)

minad mentioned this pull request May 21, 2019

Speed up travis #281

Closed

sjaeckel approved these changes May 21, 2019

View reviewed changes

minad force-pushed the restrict-v-algrind branch from efc46dd to 9d6e37e Compare May 21, 2019 09:46

sjaeckel force-pushed the restrict-v-algrind branch from 9d6e37e to 0c5a056 Compare May 21, 2019 10:09

minad force-pushed the restrict-v-algrind branch 2 times, most recently from ac030a8 to 4067696 Compare May 21, 2019 10:18

sjaeckel reviewed May 21, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

Restrict running Valgrind

ce34b3a

* Activate on the develop branch * Activate on branches containing the word v-algrind (without hyphen) * Activate if the commit message contains v-algrind (without hyphen) * Run default build always with valgrind

minad force-pushed the restrict-v-algrind branch from 32c8206 to ce34b3a Compare May 21, 2019 11:04

fix tune for non-v-algrind travis builds

01e93bb

sjaeckel force-pushed the restrict-v-algrind branch from 5d9ded8 to 01e93bb Compare May 21, 2019 11:25

run tuner only once during CI

5c7b918

v-algrind and tune builds to the beginning

30361df

... as they take the longest time

sjaeckel mentioned this pull request May 21, 2019

Tests for Karatsuba and Toom-Cook 3-way #280

Merged

COMPILE_DEBUG=1 for the valgrind build

f92d8f0

minad added 3 commits May 21, 2019 14:26

COMPILE_DEBUG should not disable optimizations

e379c0f

If you additionally want to disable optimizations, define this IGNORE_SPEED macro.

ok, one more - disable gcc mtest-vs-test runs, only run clang tests

77b4245

[skip ci]

Run gcc 4.9 build on valgrind instead of sanitized clang build

c09efff

ok, a last one - reorder stuff ;-)

f8ecaf0

[skip ci]

last last one - mtest with real rand on gcc, with prng on clang

55ce3e3

really last one - reorder once more

2555884

so all long-running jobs start in the beginning [skip ci]

sjaeckel merged commit 75d3c57 into develop May 21, 2019

sjaeckel deleted the restrict-v-algrind branch May 21, 2019 13:20

Restrict running Valgrind #282

Restrict running Valgrind #282

Uh oh!

Conversation

minad commented May 21, 2019

Uh oh!

minad commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minad commented May 21, 2019

Uh oh!

sjaeckel commented May 21, 2019

Uh oh!

minad commented May 21, 2019

Uh oh!

minad commented May 21, 2019

Uh oh!

minad commented May 21, 2019

Uh oh!

sjaeckel commented May 21, 2019

Uh oh!

minad commented May 21, 2019

Uh oh!

sjaeckel commented May 21, 2019

Uh oh!

minad commented May 21, 2019

Uh oh!

minad commented May 21, 2019

Uh oh!

sjaeckel commented May 21, 2019

Uh oh!

Uh oh!

minad commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjaeckel commented May 21, 2019

Uh oh!

sjaeckel commented May 21, 2019

Uh oh!

minad commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjaeckel commented May 21, 2019

Uh oh!

czurnieden commented May 21, 2019

Uh oh!

minad commented May 21, 2019

Uh oh!

sjaeckel commented May 21, 2019

Uh oh!

minad commented May 21, 2019

Uh oh!

sjaeckel commented May 21, 2019

Uh oh!

minad commented May 21, 2019

Uh oh!

minad commented May 21, 2019

Uh oh!

sjaeckel commented May 21, 2019

Uh oh!

minad commented May 21, 2019

Uh oh!

czurnieden commented May 21, 2019

Uh oh!

Uh oh!

minad commented May 21, 2019 •

edited

Loading

minad commented May 21, 2019 •

edited

Loading

minad commented May 21, 2019 •

edited

Loading