comparing this to current master this seems to actually be slower (if only very slightly), running debian 10.4 on an i7-8550U

bench_master.txt:SHA256, 5, 340, 4.45805, 0.0026092, 0.00263955, 0.00262045
bench_master.txt:SHA256D64_1024, 5, 7400, 4.45677, 0.000120143, 0.000120585, 0.000120541
bench_master.txt:SHA256_32b, 5, 4700000, 4.68355, 1.98788e-07, 1.99912e-07, 1.99391e-07
bench_19214.txt:SHA256, 5, 340, 4.57454, 0.00268855, 0.00269248, 0.00269149
bench_19214.txt:SHA256D64_1024, 5, 7400, 4.55315, 0.000122395, 0.000123309, 0.000123203
bench_19214.txt:SHA256_32b, 5, 4700000, 4.97084, 2.09111e-07, 2.13025e-07, 2.11958e-07

tested ACK addf18d

For me, the SHA256 tests are speeding up significantly after this.

I must have gotten something wrong, doing the benchmarks again after git clean shows this pr being about 6x faster

ACK addf18d

Can hashing be made to fail when SHA256AutoDetect hasn't been called?

On a 2019 Macbook Pro:

src/bench/bench_bitcoin -filter=SHA256.*

Before:

# Benchmark, evals, iterations, total, min, max, median
SHA256, 5, 340, 6.08992, 0.00350198, 0.00370616, 0.0035939
SHA256D64_1024, 5, 7400, 22.9059, 0.000614125, 0.00062785, 0.000618134
SHA256_32b, 5, 4700000, 6.0593, 2.55725e-07, 2.59171e-07, 2.58255e-07

After (addf18d):

# Benchmark, evals, iterations, total, min, max, median
SHA256, 5, 340, 4.12459, 0.00240666, 0.00244616, 0.00242406
SHA256D64_1024, 5, 7400, 3.56757, 9.53814e-05, 9.75219e-05, 9.61168e-05
SHA256_32b, 5, 4700000, 4.29699, 1.76951e-07, 1.91434e-07, 1.80197e-07

# Benchmark, evals, iterations, total, min, max, median
SHA256, …
SHA256D64_1024, …
SHA256_32b, …

Maybe it would be useful to specify here what SHA256 implementation is benchmarked. This makes comparisons slightly more meaningful.

From IRC:

[16:51] <phantomcircuit> sipa, oh do any of the other benchmarks maybe end up calling something that would call the auto detect?

If another benchmark spins up a testing setup, that testing setup will call auto detect. So this explains the confusing results where the naive implementation is faster than avx2.

There should probably be a way to force a specific implementation?

(I think always defaulting to the generic implementation makes sense...)

In the functional tests we use

if self.is_foobar_compiled():
  self.test_foobar()

Something along those lines could also be used to bench the different hash impls. here.

Yes, benchmarking all the different SHA356 implementations could be useful as well. I think this was the case in one of the initial PRs that introduced more of them.

That said that still leaves open what to do for other benchmarks that might depend on the SHA256 implementation. We don't want to re-run all the benchmarks for all the supported implementations ofc.

That said that still leaves open what to do for other benchmarks that might depend on the SHA256 implementation. We don't want to re-run all the benchmarks for all the supported implementations ofc.

I generally don't like using globals to magically change control flow, especially in tests. There have been enough cases in the past where global state in tests has lead to confusing results. (Including this very benchmark: #19214 (comment))

Which is why I suggested to force a decision before hashing is used: #19214 (comment) The silent fallback shouldn't be needed, or am I missing something obvious?

The silent fallback shouldn't be needed, or am I missing something obvious?

I think there's something of an initialization order issue here. Some of the objects initialized before main() might make (light, non-performance-critical) use of SHA256 to do initialization. We don't want to move the processor detection that soon due to logging / potential failure modes.

ACK addf18d
I'm going to merge this, It has enough ACKs and I think this is a clear improvement to before. Additional suggestions can be done in later PRs.