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.

Sign up for GitHub

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

Jump to bottom

Thread pool #62

Merged

alt-graph merged 27 commits into main from feature/ThreadPool

Mar 1, 2024

Member

alt-graph commented Feb 1, 2024 •

edited

Loading

This is an implementation of a ThreadPool class. We have some similar code in use in our HLC libs across multiple servers.

Some notes:

The pool always has a fixed number of threads (making that dynamic would complicate the design and I am not sure it would be worth the effort).
With 1 thread, this is a serial task queue.

Closes #28

alt-graph added the enhancement label

alt-graph requested review from soerengrunewald, Finii and ohensler

February 1, 2024 16:09

alt-graph self-assigned this

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

alt-graph force-pushed the feature/ThreadPool branch from 93583bf to 5d2868d Compare

February 2, 2024 08:58

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

alt-graph force-pushed the feature/ThreadPool branch from 5d2868d to 7ec5618 Compare

February 2, 2024 09:43

This comment was marked as outdated.

Sign in to view

Finii reviewed

View reviewed changes

Collaborator

Finii left a comment

Some thoughts on the first sequential code read ;-)

include/gul14/ThreadPool.h Outdated Show resolved Hide resolved

include/gul14/ThreadPool.h Show resolved Hide resolved

include/gul14/ThreadPool.h Outdated Show resolved Hide resolved

include/gul14/ThreadPool.h Outdated Show resolved Hide resolved

include/gul14/ThreadPool.h Outdated Show resolved Hide resolved

src/ThreadPool.cc Outdated Show resolved Hide resolved

src/ThreadPool.cc Show resolved Hide resolved

include/gul14/ThreadPool.h Outdated Show resolved Hide resolved

src/ThreadPool.cc Show resolved Hide resolved

src/ThreadPool.cc Outdated Show resolved Hide resolved

Finii reviewed

View reviewed changes

src/ThreadPool.cc Outdated Show resolved Hide resolved

Finii reviewed

View reviewed changes

include/gul14/ThreadPool.h Outdated Show resolved Hide resolved

alt-graph force-pushed the feature/ThreadPool branch 6 times, most recently from a2579db to 7b989c9 Compare

February 2, 2024 14:16

alt-graph force-pushed the feature/ThreadPool branch from 5c74dcc to 2a99446 Compare

February 9, 2024 15:44

alt-graph marked this pull request as ready for review

February 9, 2024 15:52

alt-graph force-pushed the feature/ThreadPool branch from c2aa158 to a9d5a9e Compare

February 9, 2024 16:48

alt-graph marked this pull request as draft

February 9, 2024 18:19

alt-graph force-pushed the feature/ThreadPool branch from bd4c3ce to 0e5d0ea Compare

February 9, 2024 20:53

alt-graph marked this pull request as ready for review

February 9, 2024 20:56

Member Author

alt-graph commented Feb 9, 2024

I believe this is ready for review now. The documentation looks OK, and the tests seem to pass.

Member Author

alt-graph commented Feb 12, 2024

Hold on, I guess I'll move the std::invoke-related things into their own PR and squash some of the other commits. Otherwise this is just too much...

alt-graph marked this pull request as draft

February 12, 2024 08:19

alt-graph and others added 21 commits

February 28, 2024 15:58


          ThreadPool: Add more unit tests

4f2b262

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          Fix use of invalidated iterator in condition_variable::wait_until()

3576d66

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          perform_work(): Correct index calculation

c0d8911

[why]
We first erased a vector iterator, then used it to calculate its index.
Strictly speaking, the iterator was invalidated by the erase()
and may not be used afterwards. In debug mode, Microsoft's
STL asserts on that.


          Make time-based tests more robust

8948d96

... and fix an unintentionally time-dependent test for
is_full().


          Improve documentation

1fb5dcb

Co-authored-by: Fini <ulf.fini.jastrow@desy.de>


          TaskHandle: Remove get_id()

cbd1368

[why]
The task ID is an implementation detail.

Proposed-by: Ulf Fini Jastrow <ulf.fini.jastrow@desy.de>
Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          add_task: Correct documentation of return value

2fbd993

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          Improve Doxygen comment for is_full_i()

2b5ad3a

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          Fix ThreadPool example

6d1da9c

[why]
Missing "using namespace" directive to import chrono UDLs.

Reported-by: Ulf Fini Jastrow <ulf.fini.jastrow@desy.de>
Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          CI: Try to compile examples

a3e05ec

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>


          Examples: Fix trim_sv() example

f86834f

[why]
Missing "using trim_sv" directive.

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          Merge ThreadPoolEngine back into ThreadPool

0535bd0

[why]
Having a ThreadPool class as a pure "facade" over a ThreadPoolEngine is
quite confusing.

[how]
Provide a factory function make_thread_pool() to create a ThreadPool
object in a shared_ptr, and make the ThreadPool constructor private.

Proposed-by: Ulf Fini Jastrow <ulf.fini.jastrow@desy.de>
Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          ThreadPool: Mention make_thread_pool() at start of documentation

408ed31

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          TaskHandle: Replace is_running(), is_pending() by get_state()

8e8df4d

This commit replaces the "one-trick pony" functions is_running() and
is_pending() by a quad-state get_state() function that returns an enum.

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          TaskHandle: Improve documentation & make constructor public

a646ff1

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          ThreadPool: Make TaskHandle an inline class

7de6931

[why]
This solves several visibility problems and the order of classes in the
Doxygen documentation.

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          Simplify TaskState types

12a1589

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          Improve thread pool example

29bab35

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          Doxygen: Display "topics" in addition to "modules"

f590085

[why]
We used Doxygen's \addtogroup command to define entries for our header
files. In Doxygen 1.8.17 (Ubuntu 20), this category was selected for
display with the "modules" types. Doxygen 1.9.8 has learnt about C++20
modules and reserves the "modules" type for them, so we have to use the
new "topics" keyword.

[how]
Select both "modules" and "topics" for being displayed. Doxygen 1.8.x
ignores the "topics" and Doxygen 1.9.8 does not show anything additional
for "modules" because we do not have any C++ modules in GUL14.

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          Doxygen: Fix ThreadPool documentation

dd81745

[why]
For various reasons, the documentation did not render as expected.

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>


          ThreadPool: Move TaskId into ThreadPool class

[why]
The typedef is not useful outside of the ThreadPool class.

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>

alt-graph force-pushed the feature/ThreadPool branch from 1b47f08 to 7623643 Compare

February 28, 2024 14:58

Member Author

alt-graph commented Feb 28, 2024

What do you think about having a factory function instead?

It is now implemented like that. ThreadPoolEngine is gone, we need to call make_thread_pool() instead.

alt-graph requested a review from Finii

February 28, 2024 15:20


          TaskHandle: Make get_state() tests more complete

d7ce6bf

[why]
The test only covered the possible outcomes "pending" and "running".

[how]
Add tests for "canceled" and "complete".

Signed-off-by: Lars Froehlich <lars.froehlich@desy.de>

Finii approved these changes

View reviewed changes

Collaborator

Finii left a comment

This looks good to merge!

What I do not understand is why we have

gul14::TaskState but
gul14::ThreadPool::TaskHandle

Maybe this is be accident - I think a unified solution would be easier for users to grasp.
What was the core guidline... if it does not need to access any members, keep it out of the class. But then...

I will have a look at the static/dynamic think later.

Member Author

alt-graph commented Feb 29, 2024

Thanks for the review, @Finii! I think that both architecture and usability are much better than in the original version.

What I do not understand is why we have
* `gul14::TaskState` but

* `gul14::ThreadPool::TaskHandle`

The TaskHandle needs to access a private member of the ThreadPool, and keeping it separate leads to a mess of circular dependencies between the two classes. All of that can be solved, but is very messy. The nested class was by far the best solution I found.

TaskState could go anywhere, but since this is a scoped enum, people have to write the full type name a lot:

if (handle.get_state() == ThreadPool::TaskState::complete) ...;
if (handle.get_state() == TaskState::complete) ...;

I just prefer the second one.

alt-graph mentioned this pull request

Dynamically link unit tests #83

Closed

alt-graph merged commit cb9c224 into main

3 checks passed

alt-graph deleted the feature/ThreadPool branch

March 1, 2024 11:41

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

Labels