Page MenuHomePhabricator

[ThreadPool] Support returning futures with results.
ClosedPublic

Authored by fhahn on Thu, Nov 18, 12:11 PM.

Details

Summary

This patch adjusts ThreadPool::async to return futures that wrap
the result type of the passed in callable.

To do so, ThreadPool::asyncImpl first constructs a
std::packaged_task<ResTy()> object, then extracts
the future and converts the std::packaged_task<ResTy()>
to std::packaged_task<void()> before submitting it to the
queue.

Diff Detail

Event Timeline

fhahn created this revision.Thu, Nov 18, 12:11 PM
fhahn requested review of this revision.Thu, Nov 18, 12:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Nov 18, 12:11 PM
fhahn retitled this revision from [ThreadPool] Add template argument for future result type. to [ThreadPool] Add template argument for future result type.(WIP).Thu, Nov 18, 12:18 PM
Meinersbur added a subscriber: Meinersbur.EditedThu, Nov 18, 3:12 PM

The pre-merge check has found additional locations where template arguments need to be added. Did you consider using using void as default template argument, or make ThreadPool a typedef of "ThreadPoolWithResult<void>"?

However, I don't think that the ThreadPool itself needs to be a template, but just the async implementation such that one can enqueue tasks with different return types to the same thread pool:

template <typename RetTy>
inline std::shared_future<RetTy> async(std::function<RetTy()> F);

AFAIU, std::packaged_task<> already handles return values itself.

Edit: I missed that packaged_task itself a template of the return type. However, after calling get_future, it it itself packaged into a std::function<void()> object that is pushed to the Tasks queue, i.e. does not depend on the result type anymore.

fhahn updated this revision to Diff 388437.Fri, Nov 19, 2:25 AM

The pre-merge check has found additional locations where template arguments need to be added. Did you consider using using void as default template argument, or make ThreadPool a typedef of "ThreadPoolWithResult<void>"?

Thanks for the suggestion! I updated ThreadPool -> ThreadPoolWithResult and add using ThreadPool = ThreadPoolWithResult<void>, to avoid adjusting all users.

However, I don't think that the ThreadPool itself needs to be a template, but just the async implementation such that one can enqueue tasks with different return types to the same thread pool:

template <typename RetTy>
inline std::shared_future<RetTy> async(std::function<RetTy()> F);

AFAIU, std::packaged_task<> already handles return values itself.

Edit: I missed that packaged_task itself a template of the return type. However, after calling get_future, it it itself packaged into a std::function<void()> object that is pushed to the Tasks queue, i.e. does not depend on the result type anymore.

I tried using void with both TaskTy and PackagedTaskTy, but both resulted in build errors when sharing the Future.

fhahn updated this revision to Diff 388445.Fri, Nov 19, 2:46 AM

Adjust ThreadPool forward declaration in MLIRContext.h

Herald added a project: Restricted Project. · View Herald Transcript
fhahn retitled this revision from [ThreadPool] Add template argument for future result type.(WIP) to [ThreadPool] Add template argument for future result type..Fri, Nov 19, 5:14 AM
fhahn edited the summary of this revision. (Show Details)
fhahn added reviewers: dblaikie, Meinersbur.
Meinersbur added a comment.EditedFri, Nov 19, 9:04 AM

I tried using void with both TaskTy and PackagedTaskTy, but both resulted in build errors when sharing the Future.

What were thoise build errors?

Implementing a non-templated ThreadPool should be possible, as demonstrated here.

I tried using void with both TaskTy and PackagedTaskTy, but both resulted in build errors when sharing the Future.

What were thoise build errors?

Variants of

llvm/include/llvm/Support/ThreadPool.h:184:12: error: no viable conversion from returned value of type 'shared_future<void>' to function return type 'shared_future<llvm::SmallString<0>>'
    return Future.share();
           ^~~~~~~~~~~~~~

Implementing a non-templated ThreadPool should be possible, as demonstrated here.

I think the issue is that LLVM's ThreadPool uses std::packaged_task to store the task and extract the future, and the type used for the future depends on the type for the packaged_task, which in turn depends on the type of Task.

Are you suggesting adjusting the way we package stuff in LLVM's ThreadPool so the packaged task types do not depend on the result type of the packaged function? I think that should be an feasible alternative.

This seems like a bit of a "niche" extension: it makes it possible to return future but only for a statically predefined type. Is there a way to type-erase this in the threadpool a bit better and manager this at "enqueue time" instead so that each new task can return its own future?

Also isn't this feature something that can likely be layered on top of the existing ThreadPool without changing it?
For example by wrapping the submitted task into another object that manages the promise/future.

fhahn added a comment.Fri, Nov 19, 1:44 PM

This seems like a bit of a "niche" extension: it makes it possible to return future but only for a statically predefined type. Is there a way to type-erase this in the threadpool a bit better and manager this at "enqueue time" instead so that each new task can return its own future?

Also isn't this feature something that can likely be layered on top of the existing ThreadPool without changing it?
For example by wrapping the submitted task into another object that manages the promise/future.

I think that's what Michael was asking/suggestion in the previous comment :)

My main goal is to use futures with non-void results, I'm happy to go with whatever implementation people think is most useful. Introducing a predefined type was a mostly mechanical change, but as I said if changing the packaging is preferred, I can adjust the code.

This seems like a bit of a "niche" extension: it makes it possible to return future but only for a statically predefined type. Is there a way to type-erase this in the threadpool a bit better and manager this at "enqueue time" instead so that each new task can return its own future?

Also isn't this feature something that can likely be layered on top of the existing ThreadPool without changing it?
For example by wrapping the submitted task into another object that manages the promise/future.

I think that's what Michael was asking/suggestion in the previous comment :)

Ah, I missed this! I think skimming at the link Michael provided, the ScheduleAndGetFuture is what I had in mind.

Meinersbur added a comment.EditedFri, Nov 19, 8:06 PM

Variants of

llvm/include/llvm/Support/ThreadPool.h:184:12: error: no viable conversion from returned value of type 'shared_future<void>' to function return type 'shared_future<llvm::SmallString<0>>'
    return Future.share();
           ^~~~~~~~~~~~~~

This seems to be a mismatch of asyncImpl's future return type and the return type of the function passed to asyncImpl. Future should be of type shared_future<llvm::SmallString<0>> but somehow it is shared_future<void>. That is, both should be the same template parameter, like the template<typename RetTy> std::shared_future<RetTy> async(std::function<RetTy()> F); suggested before. But this is requirements is only with the asyncImpl defintion.

More crucially, the line

Tasks.push(std::move(PackagedTask));

wraps the PackagedTask into a std::function<void()> which does not depend on the return type anymore. That is, we don't need ThreadPool::Tasks to be a template of the return type, i.e. it can store tasks of any return type (I think the actually result value is stored as a closure parameter of the std::function<void()> object, the std::future can be used to retrieve it), the ThreadPool itself does not need to store the future which was only returned by asyncImpl for the caller to remember.

fhahn updated this revision to Diff 388709.Sat, Nov 20, 10:06 AM

Updated so there's no need for adding a template argument to ThreadPool.

More crucially, the line

Tasks.push(std::move(PackagedTask));

wraps the PackagedTask into a std::function<void()> which does not depend on the return type anymore. That is, we don't need ThreadPool::Tasks to be a template of the return type, i.e. it can store tasks of any return type (I think the actually result value is stored as a closure parameter of the std::function<void()> object, the std::future can be used to retrieve it), the ThreadPool itself does not need to store the future which was only returned by asyncImpl for the caller to remember.

Thanks! It looks like it is possible to first construct a std::packaged_task<RetTy()>, extract the future and then move the object into a std::packaged_task<void()>.

It required a few changes, but it looks like it is working now without the extra template argument. It also seems async also needs to convert it's argument to a std::function object before calling asyncImpl, so asyncImpl can determine the correct template argument types for std::function.

fhahn retitled this revision from [ThreadPool] Add template argument for future result type. to [ThreadPool] Support returning futures with results..Sat, Nov 20, 10:10 AM
fhahn edited the summary of this revision. (Show Details)
fhahn updated this revision to Diff 388728.Sat, Nov 20, 12:57 PM
fhahn edited the summary of this revision. (Show Details)

attempt to work around windows build failure.

fhahn updated this revision to Diff 388729.Sat, Nov 20, 1:27 PM

another stab at trying to figure out what's going on with the windows build

fhahn updated this revision to Diff 388862.Mon, Nov 22, 3:47 AM

Replace uses of packaged_tasks with manually creating promise and callable to set result, to work around MSVC build failures.

fhahn added a comment.Mon, Nov 22, 5:13 AM

I think this should be ready for another look now. Windows tests are passing now and the failure on Debian is unrelated to the patch I think.

Unfortunately I wasn't able to find a way to convert std::packaged_task<ResTy()> to std::packaged_task<void()> with MSVC. To work around that, I updated the code to manually create a promise and wrap the task function into another callable that calls the function and updates the promise.

mehdi_amini accepted this revision.Mon, Nov 22, 11:19 AM
This revision is now accepted and ready to land.Mon, Nov 22, 11:19 AM
This revision was landed with ongoing or failed builds.Mon, Nov 22, 1:21 PM
This revision was automatically updated to reflect the committed changes.

It seems like you may have missed some issues with const vs. non-const types in the case when LLVM_ENABLE_THREADS is false.
I think it should be pretty easy to fix, but I don't know ThreadPool well enough to quickly address it myself.
This is the error:

llvm/include/llvm/Support/ThreadPool.h:127:29: error: 'this' argument to member function 'get' has type 'const std::future<void>', but function is not marked const
    Tasks.push([Future]() { Future.get(); });
                            ^~~~~~
llvm/include/llvm/Support/ThreadPool.h:60:12: note: in instantiation of function template specialization 'llvm::ThreadPool::asyncImpl<void>' requested here
    return asyncImpl(std::function<decltype(F())()>(std::forward<Func>(F)));
           ^
llvm/include/llvm/Support/ThreadPool.h:54:12: note: in instantiation of function template specialization 'llvm::ThreadPool::async<std::__bind<(lambda at llvm/lib/CodeGen/ParallelCG.cpp:80:15), llvm::SmallString<0>>>' requested here
    return async(std::move(Task));
           ^
future:1239:10: note: 'get' declared here
    void get();

Do you mind submitting a fix?

This revision is now accepted and ready to land.Wed, Nov 24, 8:14 AM
This revision now requires changes to proceed.Thu, Nov 25, 9:08 AM

It seems like you may have missed some issues with const vs. non-const types in the case when LLVM_ENABLE_THREADS is false.
I think it should be pretty easy to fix, but I don't know ThreadPool well enough to quickly address it myself.
This is the error:

llvm/include/llvm/Support/ThreadPool.h:127:29: error: 'this' argument to member function 'get' has type 'const std::future<void>', but function is not marked const
    Tasks.push([Future]() { Future.get(); });
                            ^~~~~~
llvm/include/llvm/Support/ThreadPool.h:60:12: note: in instantiation of function template specialization 'llvm::ThreadPool::asyncImpl<void>' requested here
    return asyncImpl(std::function<decltype(F())()>(std::forward<Func>(F)));
           ^
llvm/include/llvm/Support/ThreadPool.h:54:12: note: in instantiation of function template specialization 'llvm::ThreadPool::async<std::__bind<(lambda at llvm/lib/CodeGen/ParallelCG.cpp:80:15), llvm::SmallString<0>>>' requested here
    return async(std::move(Task));
           ^
future:1239:10: note: 'get' declared here
    void get();

Do you mind submitting a fix?

Your revert breaks normal building: http://45.33.8.238/linux/61593/step_4.txt

PTAL.

Your revert breaks normal building: http://45.33.8.238/linux/61593/step_4.txt

PTAL.

Ooops, sorry. Working on it

I recommitted the patch. for now and also submitted a fix for the build failure, which was caused by the return type of the future having been changed from auto to std::future bc41653a1f289a63a1472b7364437515c9659f77. Everything should build again now.

Could you close the review again if done?

fhahn accepted this revision.Mon, Nov 29, 1:54 PM

Could you close the review again if done?

I'm not sure how unfortunately :( I can't see an option to close at the moment.

Meinersbur accepted this revision.Mon, Nov 29, 2:11 PM

Could you close the review again if done?

I'm not sure how unfortunately :( I can't see an option to close at the moment.

You can only "Abandon Revision" in the list of actions at the bottom.

This revision is now accepted and ready to land.Mon, Nov 29, 2:19 PM

Actually the "Close Revision" was hidden because of the "requested changes" from Daniel, you should see it now

I try to make it disappear from my Phabricator dashboard. Seems the only way is to resign as a reviewer :-(

Macro thisisfine:

fhahn closed this revision.Tue, Nov 30, 10:11 AM