Skip to content

Add support for other multi-threading APIs#1027

Closed
krzikalla wants to merge 1 commit intogoogle:masterfrom
krzikalla:manual_threading
Closed

Add support for other multi-threading APIs#1027
krzikalla wants to merge 1 commit intogoogle:masterfrom
krzikalla:manual_threading

Conversation

@krzikalla
Copy link
Contributor

This PR replaces the OpenMP PR. It adds more flexibility to multi-threaded benchmarks.
You can perform scalability tests independent of std::thread.
And by the means of ThreadState you can also open a parallel region before the iteration loop, which enables you to measure the asynchronous progress of multiple threads across iterations.
For more information see the section "Multithreaded Benchmarks" in the README.

@google-cla google-cla bot added the cla: yes label Aug 24, 2020
{
ThreadManager::Result& results = manager->results;
results.cpu_time_used += timer->cpu_time_used();
results.real_time_used = std::max(results.real_time_used, timer->real_time_used());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to myself: leftover of another fix.
Here it still must be results.real_time_used += timer->real_time_used();

Copy link
Member

@dmah42 dmah42 left a comment

Choose a reason for hiding this comment

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

some style notes while i work through the ramifications.

can you provide some example output in the PR so i can see if there's any impact?
also we'll need some tests.

int num_thread_states_;

friend struct internal::BenchmarkInstance;
friend class ThreadState;
Copy link
Member

Choose a reason for hiding this comment

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

avoid friends please. add a protected section with accessors if you need that.

};

class ThreadState : public State
{
Copy link
Member

Choose a reason for hiding this comment

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

note we use google style so this (and other braces) should be on the previous line. you can use clang-format to make this easier.

if (st.GetNumThreadStates() > 0)
{
CHECK((!b->explicit_threading) || b->manual_threading)
<< "Benchmark " << b->name.str() << " run with managed threading. It must not create ThreadStates!";
Copy link
Member

Choose a reason for hiding this comment

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

i believe this line is too long. please check with clang-format.

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.

Thanks. This looks more acceptable.
I think this needs some tests.

BENCHMARK(BM_MultiThreaded)->ManualThreading()->Threads(1)->Threads(2)->Threads(4);
```
If you use the `ThreadState` object and explicitly specify the number of threads, then you must
use `ManualThreading` and the number of created `ThreadState` objects must match the number of specified threads.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this postcondition actually required? Can it be lifted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the postcondition, that number of ThreadStates == number of threads? I don't see a way to do this.
Note, that this postcondition is only requried, if you explicitly specify the number of threads via Threads() and if you create at least one ThreadState object during the benchmark run. It would be odd, if in this case the numbers doesn't match.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean the postcondition, that number of ThreadStates == number of threads?

Yes.

I don't see a way to do this.
Note, that this postcondition is only requried, if you explicitly specify the number of threads via Threads() and if you create at least one ThreadState object during the benchmark run. It would be odd, if in this case the numbers doesn't match.

In particular, as discussed (?) in previous PR, i would like to call ->Threads(),
but disable benchmark-based threading. I.e. i'd like a way to use threads
as yet another parameter, without it actually creating all those threads,
and without dividing the measurements by the "thread count".

@krzikalla
Copy link
Contributor Author

krzikalla commented Aug 24, 2020

can you provide some example output in the PR so i can see if there's any impact?

What exactly do you want to see?

also we'll need some tests.

Can I expect OpenMP availability, so that we can test with OpenMP as the alternative threading API? (question goes also @LebedevRI )

@dmah42
Copy link
Member

dmah42 commented Aug 24, 2020

can you provide some example output in the PR so i can see if there's any impact?

What exactly do you want to see?

what the output (tabular is fine) looks like when using openmp.

also we'll need some tests.

Can I expect OpenMP availability, so that we can test with OpenMP as the alternative threading API? (question goes also @LebedevRI )

i'm pretty sure you could check for openmp availability before enabling those tests... and add them to the buildbots.

@LebedevRI
Copy link
Collaborator

can you provide some example output in the PR so i can see if there's any impact?

What exactly do you want to see?

what the output (tabular is fine) looks like when using openmp.

also we'll need some tests.

Can I expect OpenMP availability, so that we can test with OpenMP as the alternative threading API? (question goes also @LebedevRI )

i'm pretty sure you could check for openmp availability before enabling those tests... and add them to the buildbots.

While that should work, i would personally maybe suggest just sticking with using std::thread for those tests.
It won't really matter what is used in tests.

@dmah42
Copy link
Member

dmah42 commented Aug 25, 2020

can you provide some example output in the PR so i can see if there's any impact?

What exactly do you want to see?

what the output (tabular is fine) looks like when using openmp.

also we'll need some tests.

Can I expect OpenMP availability, so that we can test with OpenMP as the alternative threading API? (question goes also @LebedevRI )

i'm pretty sure you could check for openmp availability before enabling those tests... and add them to the buildbots.

While that should work, i would personally maybe suggest just sticking with using std::thread for those tests.
It won't really matter what is used in tests.

that's true. as long as the api is being exercised and the expectations met, it shouldn't matter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants