Refactor future implementation #4270

aurianer · 2019-12-09T13:19:13Z

Add the futures files inside the execution module

Remove some dependencies
Move thread_timed_execution.hpp back in hpx/parallel/executors to avoid a circular dependency between the execution and local_lcos module

Goes on top of #4247

hkaiser · 2019-12-09T20:13:43Z

I'm not sure if this is the right granularity. In comparison with the other modules this one seems to be very large. I think the future type itself belongs into the local_lcos module (with the exception for the future_data<id_type> specialization). This would allow to step by step move things like (local) async into their own modules. For the component related files - I think we first should extract the file related to actions and AGAS, and only afterwards anything related to components. Actions and AGAS are unfortunately inseparable, but components can be built on top of those (I think).

aurianer · 2019-12-10T09:15:07Z

I'm not sure if this is the right granularity. In comparison with the other modules this one seems to be very large. I think the future type itself belongs into the local_lcos module (with the exception for the future_data<id_type> specialization). This would allow to step by step move things like (local) async into their own modules. For the component related files - I think we first should extract the file related to actions and AGAS, and only afterwards anything related to components. Actions and AGAS are unfortunately inseparable, but components can be built on top of those (I think).

The first thing is that the futures and the execution traits are very tight together so they kind of belong together, and putting futures in local_lcos would create a circular dependency between execution and local_lcos
Concerning the fact that this module is too big, I totally agree for now but we planned to put the executors in their own module and deprecate the execution/parallel/traits/detail/vc/ files. So at the end I would think the size is fine, what do you think ?
Last thing, I'm not sure I understand your reference to the component related files cause I'm not doing anything related to that in this PR ^^

hkaiser · 2019-12-13T20:44:09Z

The first thing is that the futures and the execution traits are very tight together so they kind of belong together, and putting futures in local_lcos would create a circular dependency between execution and local_lcos

I believe we can break this circular dependency by moving only the basic future to local_lcos, leaving all things related to executors separated in the executors module. This involves separating the template specialization for executors into a separate file, however.

Concerning the fact that this module is too big, I totally agree for now but we planned to put the executors in their own module and deprecate the execution/parallel/traits/detail/vc/ files. So at the end I would think the size is fine, what do you think ?

ok

Last thing, I'm not sure I understand your reference to the component related files cause I'm not doing anything related to that in this PR ^^

Please ignore this comment, I did misunderstand what you were doing here.

msimberg · 2019-12-16T10:00:45Z

I believe we can break this circular dependency by moving only the basic future to local_lcos, leaving all things related to executors separated in the executors module. This involves separating the template specialization for executors into a separate file, however.

Good point. How much of future do you think we can move to local_lcos though? Do you specifically mean future_base (the is_future etc. traits are ok too)? The implementations of future and shared_future eventually need the executor then customization point and launch policies, which are both undoubtedly execution-y. packaged_continuation definitely needs the execution module. Do you see a way to fully define hpx::future in local_lcos without depending on the the execution module? Templating future and shared_future on those implementation details is definitely one possibility but that will show up in the types (don't know if that's a bad thing).

msimberg · 2020-01-29T14:01:32Z

@hkaiser this is still slightly in progress, but I think the general changes required for the future/execution are in place and your feedback on it would be appreciated.

hpx/lcos/future.hpp

hkaiser

Very nice, thanks a lot!

hpx/lcos/future.hpp

msimberg · 2020-04-01T12:34:26Z

It looks like this is working quite well with decltype(auto) now (the GitHub Actions builders are over at @aurianer's fork, but they seem to be building this just fine also on Windows). The pycicle builder just needs to have that one header fixed. @hkaiser are you happy with this solution (assuming the test results end up greenish)?

msimberg · 2020-04-02T06:49:51Z

I suggest we go ahead with this as it is. It might still need some cleanup but exactly what to clean up will be more apparent once files get moved to their modules, which would be the next step.

To eliminate a circular dependency between local_lcos and the execution module. We include now thread_timed_executors inside the global executor include

The goal is to move the template specialization related to the executors into the execution module, the first step is to separate them but keep it in the same file

We move the template specializations related to the executors. Change the future.hpp include to the future_exec.hpp one when necessary.

We remove the dependency to executors and launch_policy.

To move the dependency to executors.

In order to avoid an additional call

It uses a future_serialize, that can send exception, we get errors without this include (the exception cannot be serialized).

aurianer · 2020-04-02T15:43:15Z

I just rebased this :)

hkaiser

LGTM, let's go ahead with this. There are still some minor questions whether we can further clean up this code, but we can do it over time. Thanks Auriane!

aurianer force-pushed the module_futures branch from e2945a1 to ef6144c Compare December 9, 2019 13:22

aurianer added the category: modules label Dec 9, 2019

aurianer added this to the 1.5.0 milestone Dec 9, 2019

aurianer added this to TODO in HPX modularization via automation Dec 9, 2019

aurianer force-pushed the module_futures branch 3 times, most recently from b16a8e4 to ed1ac69 Compare December 9, 2019 17:25

aurianer force-pushed the module_futures branch 3 times, most recently from 00e2569 to 334301b Compare December 13, 2019 14:56

aurianer force-pushed the module_futures branch 3 times, most recently from 39e39eb to 028a161 Compare December 19, 2019 10:17

aurianer force-pushed the module_futures branch from 028a161 to 698d85c Compare January 9, 2020 14:17

aurianer force-pushed the module_futures branch 4 times, most recently from 4d77d68 to c21d84b Compare January 28, 2020 15:34

msimberg requested a review from hkaiser January 29, 2020 13:38

hkaiser reviewed Jan 29, 2020

View reviewed changes

hpx/lcos/future.hpp Show resolved Hide resolved

HPX modularization automation moved this from TODO to In Progress Jan 29, 2020

hkaiser previously approved these changes Jan 29, 2020

View reviewed changes

aurianer dismissed hkaiser’s stale review via 311137a January 30, 2020 15:57

aurianer force-pushed the module_futures branch 2 times, most recently from edae5c1 to 172ee82 Compare January 30, 2020 17:09

aurianer force-pushed the module_futures branch from 0aefb09 to b1bd8ee Compare March 18, 2020 17:05

aurianer force-pushed the module_futures branch 3 times, most recently from 88918fd to ee30804 Compare March 31, 2020 19:03

msimberg reviewed Apr 1, 2020

View reviewed changes

hpx/lcos/future.hpp Show resolved Hide resolved

aurianer added 18 commits April 2, 2020 17:40

Remove lcos_fwd include from the hpx/lcos/future.hpp

17ff589

Move thread_timed_execution.hpp back in hpx/

4baa793

To eliminate a circular dependency between local_lcos and the execution module. We include now thread_timed_executors inside the global executor include

Fix docs errors

513170b

Remove unsused header

bff3579

Move the future_then_dispatch specialization at the bottom

f482389

The goal is to move the template specialization related to the executors into the execution module, the first step is to separate them but keep it in the same file

Add a dummy future_then_dispatch that fails compilation

c77d5c6

Move the executor template specializations

0f3c7cb

We move the template specializations related to the executors. Change the future.hpp include to the future_exec.hpp one when necessary.

Change the then specialization remove the dependencies

9cde21d

We remove the dependency to executors and launch_policy.

Split future_then_result into 2 files

91ea718

To move the dependency to executors.

Add a dummy implementation for call_alloc

52f5bfd

Remove unused includes

0a33f64

Make the global future include with executors overloads

df2a763

Change the dummy_false_type to sizeof(Future) == 0

c9c77e7

In order to avoid an additional call

Remove the remaining is_executor include

602bf5b

Make future_then_result instantiation lazy

ab1a38e

fixup! Change the then specialization remove the dependencies

68adf05

Use decltype(auto) for the other then overloads

0d5c64d

Add serialize_exception include back

e8c0e73

It uses a future_serialize, that can send exception, we get errors without this include (the exception cannot be serialized).

aurianer force-pushed the module_futures branch from 2f03ba1 to e8c0e73 Compare April 2, 2020 15:43

hkaiser approved these changes Apr 4, 2020

View reviewed changes

msimberg merged commit 1d221a2 into STEllAR-GROUP:master Apr 6, 2020

HPX modularization automation moved this from In Progress to Done Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor future implementation #4270

Refactor future implementation #4270

aurianer commented Dec 9, 2019 •

edited

hkaiser commented Dec 9, 2019

aurianer commented Dec 10, 2019 •

edited

hkaiser commented Dec 13, 2019 •

edited

msimberg commented Dec 16, 2019

msimberg commented Jan 29, 2020

hkaiser left a comment

msimberg commented Apr 1, 2020

msimberg commented Apr 2, 2020

aurianer commented Apr 2, 2020

hkaiser left a comment

Refactor future implementation #4270

Refactor future implementation #4270

Conversation

aurianer commented Dec 9, 2019 • edited

hkaiser commented Dec 9, 2019

aurianer commented Dec 10, 2019 • edited

hkaiser commented Dec 13, 2019 • edited

msimberg commented Dec 16, 2019

msimberg commented Jan 29, 2020

hkaiser left a comment

Choose a reason for hiding this comment

msimberg commented Apr 1, 2020

msimberg commented Apr 2, 2020

aurianer commented Apr 2, 2020

hkaiser left a comment

Choose a reason for hiding this comment

aurianer commented Dec 9, 2019 •

edited

aurianer commented Dec 10, 2019 •

edited

hkaiser commented Dec 13, 2019 •

edited