RFC: bench: Add multi thread benchmarking #33740
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33740. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
I people could test the exact command as above it might be interesting to post the results here. |
fjahr
force-pushed
the
202510-multi-thread-bench
branch
from
e2806ef to
de25d1a
Compare
l0rinc
left a comment
l0rinc
left a comment
There was a problem hiding this comment.
Not sure whether the concept of script validation has to creep into the benchmarking parameters, maybe we could generalize it to -thread-count or -par or something. Left a few nits.
| if (threads > 0) { | ||
| bool found = false; | ||
| for (auto& arg : current_setup_args) { | ||
| if (arg.starts_with("-worker-threads=") == 0) { |
There was a problem hiding this comment.
hmmm, this looks like a refactor gone wrong, was this arg.find("-worker-threads=") == 0 originally?
| if (arg.starts_with("-worker-threads=") == 0) { | |
| if (arg.starts_with("-worker-threads=")) { |
so I guess if (!found) { should also be adjusted after this
Alternatively, instead of finding and overwriting, what if we unconditionally erased and pushed back the actual thread count, something like:
std::vector current_setup_args {args.setup_args};
if (threads > 0) {
std::erase_if(current_setup_args, [](const auto& arg) { return arg.starts_with("-worker-threads="); });
current_setup_args.push_back(strprintf("-worker-threads=%d", threads));
}| // Scale from 1 to MAX_SCRIPTCHECK_THREADS | ||
| constexpr int MAX_SCRIPTCHECK_THREADS = 15; | ||
| for (int i = 1; i <= MAX_SCRIPTCHECK_THREADS; ++i) { | ||
| thread_scales.push_back(i); | ||
| } |
There was a problem hiding this comment.
Script checks are currently the main source of threading, but not necessarily limited to this (e.g. https://github.com/bitcoin/bitcoin/pull/31132/commits or #26966 or #33689 or #32747), so I would suggest untangling multithreading from script validation here. If you insist this being about script validation (which sounds a bit too specific to include in a general benchmark), ignore the comment.
nit: redundant comment, explains exactly what the code already explains without adding additional context
| } else if (m_node.args->IsArgSet("-worker-threads")) { | ||
| worker_threads = m_node.args->GetIntArg("-worker-threads", 2); | ||
| if (worker_threads < 0) { | ||
| throw std::runtime_error("-worker-threads must be non-negative"); |
There was a problem hiding this comment.
What should happen when worker_threads > MAX_SCRIPTCHECK_THREADS?
| std::cout << "Warning: -scale-threads requires -filter to be set. Running with default thread count." << std::endl; | ||
| } | ||
|
|
||
| std::vector<int> thread_scales; |
There was a problem hiding this comment.
What's the purpose of a vector over a range? Are we planning on testing threads 2,4,8,16 instead of 1,2,3,4,5,6...?
If not, consider making it a range, something like:
constexpr int DEFAULT_MAX_BENCH_THREADS{16};
const auto [start_threads, end_threads]{is_thread_scaling ? std::pair{1, DEFAULT_MAX_BENCH_THREADS} : std::pair{0, 0}};and maybe iterate as
for (int threads{start_threads}; threads <= end_threads; ++threads) {
...
}| for (const auto& arg : args.setup_args) ret.emplace_back(arg.c_str()); | ||
| return ret; | ||
| }; | ||
| bool is_thread_scaling = args.scale_threads && (args.regex_filter != ".*"); |
There was a problem hiding this comment.
Might be out of scope for this PR, but instead of inlining DEFAULT_BENCH_FILTER here, can we move it over to src/bench/bench.cpp
| bool is_thread_scaling = args.scale_threads && (args.regex_filter != ".*"); | |
| bool is_thread_scaling = args.scale_threads && (args.regex_filter != DEFAULT_BENCH_FILTER); |
Thanks, happy to change this and address the other comments. But aside from that do you think this is generally interesting enough to be considered merging? I was unsure and since I haven't worked much on benchmarking I would like to get some concept-ack-ish feedback from the benchmarking wg :) |
3 participants
This adds a rough way to run specific benchmarks with different numbers of worker threads to see how these impact performance. This is useful for me in the batch validation context and for potential refactorings of checkqueue but I am not sure if this is useful in many other contexts so I am leaving this as a RFC draft for now to see if there is any interest in merging something like this.
Example output: