Add classes for Task and SharedTask in threadpool #5391

ypatia · 2024-11-29T13:54:20Z

Currently ThreadPool::Task is a typedef to std:: future<Status>. This PR:

Replaces this with a full class that imposes things like wait for our async processes to go through our ThreadPool's wait method. In that way, we never use a std::future's wait method directly and we avoid possible deadlocks because only the ThreadPool's wait will yield.
Adjusts the structure and relationship of tasks and the threadpool so that the caller can rely on the task's internal functions and doesn't need to keep track of the relationship between the ThreadPool and each task manually throughout the codebase.
Adds a new first class citizen of our ThreadPool: SharedTask, that is encapsulating std::shared_future which allows multiple threads to wait on an async operation result to become available. This is needed for the upcoming work on parallelizing IO and compute operations in the codebase even further.

This is heavily influenced from similar work by @Shelnutt2.

TYPE: IMPROVEMENT
DESC: Add classes for Task and SharedTask in threadpool

ypatia · 2024-12-02T10:01:09Z

Passes CI, the MANYLINUX failure is a known issue.

tiledb/common/thread_pool/thread_pool.h

teo-tsirpanis · 2024-12-02T16:06:55Z

tiledb/sm/query/readers/dense_reader.cc

@@ -432,7 +432,7 @@ Status DenseReader::dense_read() {
    // prevent using too much memory when the budget is small and doesn't allow
    // to process more than one batch at a time.
    if (wait_compute_task_before_read && compute_task.valid()) {
-      throw_if_not_ok(resources_.compute_tp().wait(compute_task));
+      throw_if_not_ok(resources_.compute_tp().wait(&compute_task));


Is the plan to replace all calls to tp.wait(task) with task.wait()?

yes, I can do it already if you think it should be in this PR, or in the following work as I was planning.

Hmm, on the one hand I prefer the existing APIs because the action primarily happens on the thread pool instead of the task, but on the other hand there is already a wait() method on std::future that we must be careful not to call because of deadlocks. 🤔

Right, and also having to pass around a reference to the threadpool object everywhere is quite cumbersome.

tiledb/common/thread_pool/thread_pool.h

rroelke · 2024-12-12T15:51:25Z

tiledb/common/thread_pool/thread_pool.h

+    }
+
+    /**
+     * Is this task valid. Wait can only be called on vaid tasks.


Typo: valid

rroelke · 2024-12-12T15:53:15Z

tiledb/common/thread_pool/thread_pool.h

+    }
+
+    /**
+     * The encapsulated std::shared_future


The PR description mentions that it is not safe to use the wait method of this but that doesn't live in any of these comments. I think it probably should. Either that or std::future isn't really the right thing to use (in the hypothetical future state, for now it makes sense transiently)

rroelke · 2024-12-12T15:54:39Z

tiledb/common/thread_pool/thread_pool.h

+     */
+    std::future_status wait_for(
+        const std::chrono::milliseconds timeout_duration) const {
+      return f_.wait_for(timeout_duration);


This blocks, doesn't it?
So it will prevent other tasks in the thread pool from running on this thread until the duration has elapsed.

But from what I've read our thread pool doesn't really have much support for event-based task wake-ups right?

Yes it blocks. The current thread pool is pretty basic. Ideally we should evolve it (maybe transitioning to coroutines?) to be able to put back in the queue a task that is blocked waiting. This will also solve the deadlocks that are very much possible today in our system with that stacking of blocked tasks. Let's discuss more in a call if you'd like.

I'll follow up in 2025 about coroutines. They're definitely great; but hopefully we can avoid inventing a new runtime

Indeed, any more incremental a change would be welcome. I thought about multiple task queues and work stealing as well, or a way to suspend a thread when it's blocked and put it back to the single task queue that we have today, but I am not sure if/how the latter could be done without coroutines. Let's discuss further and we could also put together some document with ideas.

rroelke · 2024-12-12T20:24:43Z

tiledb/sm/query/readers/dense_reader.cc

@@ -368,7 +368,7 @@ Status DenseReader::dense_read() {
  // This is as far as we should go before implementing this properly in a task
  // graph, where the start and end of every piece of work can clearly be
  // identified.
-  ThreadPool::Task compute_task;
+  ThreadPool::SharedTask compute_task;


It's easy to see that this is waited upon multiple times - but if it worked fine before, why is it necessary for it to be shared now?

ypatia added 2 commits November 29, 2024 15:28

Threadpool changes

93f16c0

Add threadpool helper function

277272b

ypatia requested a review from Shelnutt2 November 29, 2024 13:54

ypatia force-pushed the yt/sc-59606/threadpool_with_tasks branch from 411e836 to e3aff67 Compare November 29, 2024 13:57

ypatia added 2 commits November 29, 2024 16:10

Clang format changes

e000852

Cleanup not needed changes

2d0a155

ypatia force-pushed the yt/sc-59606/threadpool_with_tasks branch from 0b11b03 to 2d0a155 Compare November 29, 2024 14:10

Add some more documentation to new classes

77b52a2

ypatia force-pushed the yt/sc-59606/threadpool_with_tasks branch 3 times, most recently from 49dfadf to 7c9acb3 Compare December 2, 2024 09:02

Default initialize and check for null threadpool in task classes

bdd4b48

ypatia force-pushed the yt/sc-59606/threadpool_with_tasks branch from 7c9acb3 to bdd4b48 Compare December 2, 2024 11:01

Shelnutt2 reviewed Dec 2, 2024

View reviewed changes

tiledb/common/thread_pool/thread_pool.h Outdated Show resolved Hide resolved

teo-tsirpanis reviewed Dec 2, 2024

View reviewed changes

Address review comments

c92a83c

ypatia force-pushed the yt/sc-59606/threadpool_with_tasks branch from e30927b to c92a83c Compare December 3, 2024 14:56

ypatia added 2 commits December 4, 2024 12:16

Address review comments round 2

1066214

Fix unit test

85dee35

Shelnutt2 approved these changes Dec 6, 2024

View reviewed changes

Merge branch 'dev' into yt/sc-59606/threadpool_with_tasks

766a8bb

ypatia merged commit 2637682 into dev Dec 9, 2024
63 checks passed

ypatia deleted the yt/sc-59606/threadpool_with_tasks branch December 9, 2024 08:36

ihnorton requested a review from rroelke December 12, 2024 15:31

rroelke reviewed Dec 12, 2024

View reviewed changes

ypatia mentioned this pull request Dec 18, 2024

Threadpool tasks: wait for futures to be ready in destructor #5410

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add classes for Task and SharedTask in threadpool #5391

Add classes for Task and SharedTask in threadpool #5391

ypatia commented Nov 29, 2024

ypatia commented Dec 2, 2024

teo-tsirpanis Dec 2, 2024

ypatia Dec 3, 2024

teo-tsirpanis Dec 3, 2024

ypatia Dec 4, 2024

rroelke Dec 12, 2024

rroelke Dec 12, 2024

rroelke Dec 12, 2024

rroelke Dec 12, 2024

ypatia Dec 13, 2024

rroelke Dec 17, 2024

ypatia Dec 17, 2024

rroelke Dec 12, 2024

Add classes for Task and SharedTask in threadpool #5391

Add classes for Task and SharedTask in threadpool #5391

Conversation

ypatia commented Nov 29, 2024

ypatia commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment