Given how much #920 slowed down the tests with low iteration count, replace it with 3 different similar test:
Is this the first test where we run essentially different checks depending on the count variable?
I’m slightly concerned that it introduces an “unexpected” element in the tests.
4816+ 0x1a, 0xee, 0xe6, 0xb7, 0x6e, 0x05, 0x3f, 0xc6
4817+ };
4818+ /* For every combination of 6 bit positions out of 256, restricted to
4819+ * 20-bit windows (i.e., the first and last bit position are no more than
4820+ * 19 bits apart), all 64 bit patterns occur in the input scalars used in
4821+ * this test. */
This condition was a little hard to understand. Do you have a script that confirms it?
Alternatively, should we replace this with the following easier-to-understand condition, which is analogous to the one below (this script confirms that it holds: https://gist.github.com/robot-dreams/02d27311448bd4cb79bec3ce155bf21a#file-verify_1049_patterns-py):
0 /* For every combination of 6 consecutive bit positions, all 64 bit
1 * patterns occur in the input scalars used in this test. */
ACK ed939ded6cbe01680086cc3d57643b075a288fba
I verified the expected hashes with an independent implementation: https://gist.github.com/robot-dreams/02d27311448bd4cb79bec3ce155bf21a
I also verified on a Linux VM that it’s indeed faster:
0# time valgrind ./tests 2
1real 4m48.751s user 3m55.599s sys 0m52.974s
2
3# time valgrind ./tests 4
4real 7m2.431s user 6m1.322s sys 1m0.990s
0# time valgrind ./tests 2
1real 11m44.887s user 10m49.579s sys 0m54.015s
2
3# time valgrind ./tests 4
4real 12m25.258s user 11m32.643s sys 0m52.434s
@real-or-random I see your concern. The way I’m thinking about it is, “this test does a bunch of ecmults and checks the result; count just controls how many ecmults are being done”.
I agree it’d be nicer if the relationship between count and the actual number (and choice) of ecmults were more direct, but I don’t think it’s critical. But if you DO think it’s critical, then as an alternative I might suggest having lower count correspond to a lower maximum value of i in the test_ecmult_constants_2bit.
@real-or-random I see your concern. The way I’m thinking about it is, “this test does a bunch of ecmults and checks the result;
countjust controls how many ecmults are being done”.
Indeed but your numbers show that it’s really significant on valgrind… Maybe for the paranoid, we should document a number where it’s guaranteed that all tests run? I guess anyway all tests should run at 64 but maybe even lower? For example, is there a reason why you choose 35? (And not 32? Sorry, I don’t want to start bikeshedding over the number, just trying to understand.)
This is indeed somewhat unusual, at least compared to what we had before, where the code paths used never depend on the count variable.
My justification here is that while the count variable now changes the test code paths, it doesn’t (or at least, shouldn’t) change the code paths being tested. Increasing count just runs it with more (fixed) inputs.
The reasoning is something like: #920 was probably overkill, but I also don’t want to throw it away for cases where we have enough time to still run it. So replace it with a significantly cheaper test that tests the same code paths (and mostly the same table entries; possibly even more), but also still run the old code if count is high enough.
If this feels all too arbitrary (which I find understandable), I’m also fine with just replacing the whole thing with only the consecutive-8-bits 2048-point test. @robot-dreams
0patterns = []
1for i in range(1, 1<<20, 2):
2 if bin(i).count("1") == 6:
3 for j in range(256):
4 if (i << j) >> 256 == 0:
5 patterns.append(i << j)
then for each pattern in patterns, it should should hold that x & pattern takes on 64 different values as x iterates over the scalars in the test.
Concept ACK
The reasoning is something like: #920 was probably overkill, but I also don’t want to throw it away for cases where we have enough time to still run it.
This was also my initial intuition and I agree. It’s not elegant but it’s better than throwing tests away. This will also be helpful for other “overkill” tests, e.g., see the discussion about an “million bytes” SHA256 test: #731 (review)
Maybe this is the time to add a -h / --help and then mention that not all tests may be run when the count is lower than the default.
For example, is there a reason why you choose 35? (And not 32)?
Specifying count=1 does ~1024 multiplications, count=3 does ~1024×3, count=35 does ~1024×35.
utACK ed939ded6cbe01680086cc3d57643b075a288fba
Fine with merging this as-is but I wonder if we can improve the “grepability” of such tests. At the moment git grep "count >" -- src/tests.c works but it’s fragile. I wonder if we can introduce a function/macro with a one-line comment to make it sure the skipped tests can be found easily.
edit: The macro could even print a notice like “Skipping test x due to the low iteration count.”
ACK 070e772211b3fcd297577b90b56bbf7a5cfbd0a3, the addition of the CONDITIONAL_TEST macro is nice.
I also searched for \<count\> in tests.c just to make sure there isn’t anything else that should use the macro. The only possible case I found was this one, but I’m okay with leaving it as is because it would always still run at least once (count must be positive).