cc @martinus @JeremyRubin

Hmmmm...

I'd maybe rather just abort the test and return early if only 1 thread gets made because we shouldn't ever be running with the checkqueue on a single core machine...

@JeremyRubin

Hmmmm...

I'd maybe rather just abort the test and return early if only 1 thread gets made because we shouldn't ever be running with the checkqueue on a single core machine...

We do not know for sure who many CPU cores/threads users dedicate to Bitcoin Core :)

With this PR benchmark results are quite consistent:

$ taskset --cpu-list 0-7 src/bench/bench_bitcoin -filter=CCheckQueueSpeedPrevectorJob
|              ns/job |               job/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|              177.62 |        5,629,902.65 |    1.9% |      0.01 | `CCheckQueueSpeedPrevectorJob`

$ taskset --cpu-list 0-3 src/bench/bench_bitcoin -filter=CCheckQueueSpeedPrevectorJob
|              ns/job |               job/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|              176.05 |        5,680,327.83 |    3.3% |      0.01 | `CCheckQueueSpeedPrevectorJob`

$ taskset --cpu-list 0-1 src/bench/bench_bitcoin -filter=CCheckQueueSpeedPrevectorJob
|              ns/job |               job/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|               55.45 |       18,034,367.56 |    2.6% |      0.00 | `CCheckQueueSpeedPrevectorJob`

$ taskset --cpu-list 0 src/bench/bench_bitcoin -filter=CCheckQueueSpeedPrevectorJob
|              ns/job |               job/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|               54.73 |       18,271,118.07 |    4.0% |      0.00 | `CCheckQueueSpeedPrevectorJob`

And early return do not disable the test, right?

If it's a benchmark that benchmarks thread synchronization/interaction (which is, I think, the only valid reason to run a benchmark multi-threaded) then I agree running it without threads makes little sense.

(And if so you could even see the "two threads on a single-core machine" case as a realistic case, because bitcoind will create threads on such a machine)

Updated d786765 -> a65c1ad (pr19710.01 -> pr19710.02, diff).

Addressed:

• @JeremyRubin's comment:

Hmmmm...

I'd maybe rather just abort the test and return early if only 1 thread gets made because we shouldn't ever be running with the checkqueue on a single core machine...

• @laanwj's comment

If it's a benchmark that benchmarks thread synchronization/interaction (which is, I think, the only valid reason to run a benchmark multi-threaded) then I agree running it without threads makes little sense.

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• refactor: Make CCheckQueue RAII-styled #18731 (refactor: Make CCheckQueue RAII-styled by hebasto)
• Add local thread pool to CCheckQueue #18710 (Add local thread pool to CCheckQueue by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

cr ACK a65c1ad

Updated a65c1ad -> 4f91643 (pr19710.02 -> pr19710.03, diff).

Addressed @vasild's comments:

• bench: Prevent thread oversubscription and decreases the variance of result values #19710 (comment):

Also, maybe don't drop the MIN_CORES constant and use it here instead of the magic number "2"?

• bench: Prevent thread oversubscription and decreases the variance of result values #19710 (comment)

Maybe also comment here "why we do - 1".

Updated 4f91643 -> 5e7acd6 (pr19710.03 -> pr19710.04, diff).

Addressed @promag's comment:

nit, single line and also add comment.

Code review ACK 5e7acd6.

@MarcoFalke Mind having another look at this PR?

Updated 5e7acd6 -> 3edc4e3 (pr19710.04 -> pr19710.05, diff).

Addressed @promag's comment:

Maybe ditch MIN_CORES and change this to GetNumCores() == 1?

Code review ACK 3edc4e3

ACK