Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for other multi-threading APIs #1667

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

krzikalla
Copy link

Support the benchmarking of code, which relies on other multi-threading APIs, e.g. OpenMP.

krzikalla and others added 3 commits August 25, 2020 09:55
Support the benchmarking of code, which relies on other
multi-threading APIs, e.g. OpenMP.
@krzikalla
Copy link
Author

krzikalla commented Sep 20, 2023

This is a follow-up to PR #1027
The approach is basically the same, it now addresses most of the comments made there.

@LebedevRI
Copy link
Collaborator

I guess i will need to review this. It's unfortunate that so much time has passed
since the previous attempt, i basically do not recall what specific issues i had there.

krzikalla and others added 12 commits October 5, 2023 14:00
a usual State object instead of copying it. This fixes
a bug, which otherwise happens, if one thread has already
finished the benchmark loop and merged its results to the
parent state, while another one hasn't started yet.
Support the benchmarking of code, which relies on other
multi-threading APIs, e.g. OpenMP.
a usual State object instead of copying it. This fixes
a bug, which otherwise happens, if one thread has already
finished the benchmark loop and merged its results to the
parent state, while another one hasn't started yet.
@krzikalla
Copy link
Author

I guess i will need to review this. It's unfortunate that so much time has passed since the previous attempt, i basically do not recall what specific issues i had there.

Any chance to get a review on this soon? Do I have to do something before you start? The issues I still have in the CI runners don't seem to be related to my additions.

@dmah42 dmah42 requested a review from LebedevRI October 17, 2023 15:28
Copy link
Collaborator

@LebedevRI LebedevRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess i will need to review this. It's unfortunate that so much time has passed since the previous attempt, i basically do not recall what specific issues i had there.

Any chance to get a review on this soon?

Wouldn't that be nice :(

Do I have to do something before you start?

No.

Comment on lines +1299 to +1301
// Don't create threads. Let the user evaluate state.threads and/or use
// ThreadState.
Benchmark* ManualThreading() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This presumably should assert that thread_counts_ is empty.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually fine to have a non-empty thread_counts_. In that case the benchmark is executed with each thread count and the user is responsible for creating the appropriate number of threads. The first two examples in the user guide use this combination. Later src/benchmark_runner.cc:140 checks, if the thread count and the number of ThreadState created match.

@@ -1274,6 +1296,13 @@ class BENCHMARK_EXPORT Benchmark {
// Equivalent to ThreadRange(NumCPUs(), NumCPUs())
Benchmark* ThreadPerCpu();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These all should probably assert !manual_threading_.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said above, it is fine to combine these functions with manual threading.

@@ -66,6 +69,9 @@ class BenchmarkInstance {
double min_warmup_time_;
IterationCount iterations_;
int threads_; // Number of concurrent threads to us
bool manual_threading_;
bool explicit_threading_; // true: Number of threads come from a Threads()
// call
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't fully looked at things, but preliminary, i don't really like the names chosen.

src/benchmark_api_internal.h Show resolved Hide resolved
@dmah42 dmah42 added the incomplete work needed label Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incomplete work needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants