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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement an API for asynchronous pack traversal #2829

Merged
merged 12 commits into from Oct 13, 2017

Conversation

Projects
None yet
4 participants
@Naios
Contributor

Naios commented Aug 13, 2017

This is the third PR of my GSoC 馃尀 project

It implements an API for traversing a pack in an asynchronous way.

It uses exceptions for cancelling the current control flow, hoping this is available on all platforms (nvcc).

I also added the when_each test to the build since it wasn't included, also there wasn't any indication that it was excluded from the build for any reason.

  • Unify the shared state creation

@hkaiser hkaiser added this to the 1.1.0 milestone Aug 13, 2017

@Naios Naios changed the title from Implement an API for asynchronous pack traversal to WIP Implement an API for asynchronous pack traversal Aug 14, 2017

@Naios Naios changed the title from WIP Implement an API for asynchronous pack traversal to [WIP] Implement an API for asynchronous pack traversal Aug 14, 2017

@Naios Naios changed the title from [WIP] Implement an API for asynchronous pack traversal to Implement an API for asynchronous pack traversal Aug 14, 2017

@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Aug 14, 2017

Contributor

@hkaiser I removed the usage of exceptions as discussed

Contributor

Naios commented Aug 14, 2017

@hkaiser I removed the usage of exceptions as discussed

@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Aug 20, 2017

Contributor

@hkaiser The commit which converts when_all for using this API is stashed at Naios@142ce25 . I'll create a separate PR after this was merged.

Contributor

Naios commented Aug 20, 2017

@hkaiser The commit which converts when_all for using this API is stashed at Naios@142ce25 . I'll create a separate PR after this was merged.

@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Aug 21, 2017

Contributor

I added the internal transition of dataflow and when_all to the async pack traversal API to this PR.

Contributor

Naios commented Aug 21, 2017

I added the internal transition of dataflow and when_all to the async pack traversal API to this PR.

@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Aug 23, 2017

Contributor

Because of the cyclic references: I think those are occurring because the future_data object for the returned dataflow and when_all future is carrying the future arguments even after the dataflow has finished (because the process is allocation optimized). Also this means that the current implementation of when_all is still prone for cyclic references with shared_future objects involved.

I'll add a correction commit for this soon.

But probably the better way of fixing this is to release the reference of shared_futures correctly if the object was moved out.

Contributor

Naios commented Aug 23, 2017

Because of the cyclic references: I think those are occurring because the future_data object for the returned dataflow and when_all future is carrying the future arguments even after the dataflow has finished (because the process is allocation optimized). Also this means that the current implementation of when_all is still prone for cyclic references with shared_future objects involved.

I'll add a correction commit for this soon.

But probably the better way of fixing this is to release the reference of shared_futures correctly if the object was moved out.

@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Aug 27, 2017

Contributor

@K-ballo I tracked the mentioned reset part in dataflow back to your commit: 10d4604#diff-766605f99e96cd27825878acea1198c5R56.
Do you remind the reason of this change?

Since the future arguments of dataflow and when_all are stored inside the traversal frame and later moved out, I assume it's safe to remove this part in the new dataflow backend, because the references to the passed futures is removed through this way.

Contributor

Naios commented Aug 27, 2017

@K-ballo I tracked the mentioned reset part in dataflow back to your commit: 10d4604#diff-766605f99e96cd27825878acea1198c5R56.
Do you remind the reason of this change?

Since the future arguments of dataflow and when_all are stored inside the traversal frame and later moved out, I assume it's safe to remove this part in the new dataflow backend, because the references to the passed futures is removed through this way.

@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Aug 27, 2017

Contributor

@hkaiser From my side this PR is ready for merging now

Contributor

Naios commented Aug 27, 2017

@hkaiser From my side this PR is ready for merging now

@K-ballo

This comment has been minimized.

Show comment
Hide comment
@K-ballo

K-ballo Aug 28, 2017

Member

@K-ballo I tracked the mentioned reset part in dataflow back to your commit: 10d4604#diff-766605f99e96cd27825878acea1198c5R56.
Do you remind the reason of this change?

That commit isn't changing the preexisting reset behavior, just adapting it to make it work with the newly introduced boost::reference_wrapper support. For any other kind of "Future" the reset behavior remains the same, which means it gets assigned from a default constructed instance.

Member

K-ballo commented Aug 28, 2017

@K-ballo I tracked the mentioned reset part in dataflow back to your commit: 10d4604#diff-766605f99e96cd27825878acea1198c5R56.
Do you remind the reason of this change?

That commit isn't changing the preexisting reset behavior, just adapting it to make it work with the newly introduced boost::reference_wrapper support. For any other kind of "Future" the reset behavior remains the same, which means it gets assigned from a default constructed instance.

@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Sep 6, 2017

Contributor

Also this should fix #1126

Contributor

Naios commented Sep 6, 2017

Also this should fix #1126

struct relocate_index_pack;
template <std::size_t Offset, std::size_t... Sequence>
struct relocate_index_pack<Offset, pack_c<std::size_t, Sequence...>>
: std::common_type<pack_c<std::size_t, (Sequence + Offset)...>>

This comment has been minimized.

@sithhell

sithhell Sep 20, 2017

Member

Why do we need common_type here? The pack is expanded inside of pack_c, this looks redundant.

@sithhell

sithhell Sep 20, 2017

Member

Why do we need common_type here? The pack is expanded inside of pack_c, this looks redundant.

This comment has been minimized.

@Naios

Naios Sep 20, 2017

Contributor

Usually I'm using common_type as a shorthand alias to provide a type alias from the object.
Since common_type with a single type is redundant to:

template<typename T>
struct my_type {
  using type = T;
};

Theoretically I could also add a type alias to the object itself.

@Naios

Naios Sep 20, 2017

Contributor

Usually I'm using common_type as a shorthand alias to provide a type alias from the object.
Since common_type with a single type is redundant to:

template<typename T>
struct my_type {
  using type = T;
};

Theoretically I could also add a type alias to the object itself.

This comment has been minimized.

@K-ballo

K-ballo Sep 20, 2017

Member

Not that it matters in this particular instance, but that should be:

template<typename T>
struct type {
  using type = std::decay_t<T>;
};

There doesn't seem to be anything traversal specific to this. Should this functionality be in util/detail/pack.hpp instead?

@K-ballo

K-ballo Sep 20, 2017

Member

Not that it matters in this particular instance, but that should be:

template<typename T>
struct type {
  using type = std::decay_t<T>;
};

There doesn't seem to be anything traversal specific to this. Should this functionality be in util/detail/pack.hpp instead?

This comment has been minimized.

@Naios

Naios Sep 20, 2017

Contributor

@K-ballo yes, maybe we could also move this to util/detail/pack.hpp since its not related to the traversal itself.

EDIT: Ah and you are right. I've totally overseen that common_type decays the type.

@Naios

Naios Sep 20, 2017

Contributor

@K-ballo yes, maybe we could also move this to util/detail/pack.hpp since its not related to the traversal itself.

EDIT: Ah and you are right. I've totally overseen that common_type decays the type.

}
/// We require a virtual base
~async_traversal_frame() noexcept override {}

This comment has been minimized.

@sithhell

sithhell Sep 20, 2017

Member

Do all our compilers support override? At the very least, this needs to be checked during cmake configuration.

@sithhell

sithhell Sep 20, 2017

Member

Do all our compilers support override? At the very least, this needs to be checked during cmake configuration.

This comment has been minimized.

@Naios

Naios Sep 20, 2017

Contributor

Yes all compilers support it. But you are right, the feature test for it is missing in CMake and should be added.

@Naios

Naios Sep 20, 2017

Contributor

Yes all compilers support it. But you are right, the feature test for it is missing in CMake and should be added.

point.async_traverse(std::forward<Child>(child));
// Propagate the control detach back
if (point.is_detached())

This comment has been minimized.

@sithhell

sithhell Sep 20, 2017

Member

How can a newly created frame be in detached state?

@sithhell

sithhell Sep 20, 2017

Member

How can a newly created frame be in detached state?

This comment has been minimized.

@Naios

Naios Sep 20, 2017

Contributor

This isn't related to a future frame.

The control here is detached if the visited child element detached the control flow.
This happens for instance if we visit a sequence of futures:

tuple<future<void>, std::vector<future<int>>>

and reach the child std::vector<future<int>> of the tuple and one of it's containing futures isn't ready and we have to wait for its completion asynchronously.

@Naios

Naios Sep 20, 2017

Contributor

This isn't related to a future frame.

The control here is detached if the visited child element detached the control flow.
This happens for instance if we visit a sequence of futures:

tuple<future<void>, std::vector<future<int>>>

and reach the child std::vector<future<int>> of the tuple and one of it's containing futures isn't ready and we have to wait for its completion asynchronously.

This comment has been minimized.

@sithhell

sithhell Sep 20, 2017

Member

Ah, you are correct, I missed that we are doing a async_traverse before checking, thanks for the clarification.

@sithhell

sithhell Sep 20, 2017

Member

Ah, you are correct, I missed that we are doing a async_traverse before checking, thanks for the clarification.

@K-ballo

This comment has been minimized.

Show comment
Hide comment
@K-ballo

K-ballo Oct 9, 2017

Member

@Naios would you please rebase?

Member

K-ballo commented Oct 9, 2017

@Naios would you please rebase?

@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Oct 9, 2017

Contributor

@K-ballo /done. Let me know, if there is anything missing for this PR please.

Contributor

Naios commented Oct 9, 2017

@K-ballo /done. Let me know, if there is anything missing for this PR please.

@K-ballo

This comment has been minimized.

Show comment
Hide comment
@K-ballo

K-ballo Oct 10, 2017

Member

@Naios we are still missing a feature-test for override, but that can be in its own branch (as long as we merge it first).

Member

K-ballo commented Oct 10, 2017

@Naios we are still missing a feature-test for override, but that can be in its own branch (as long as we merge it first).

@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Oct 10, 2017

Contributor

@K-ballo I'll provide the feature test as a separate PR.

Contributor

Naios commented Oct 10, 2017

@K-ballo I'll provide the feature test as a separate PR.

Naios added a commit to Naios/hpx that referenced this pull request Oct 10, 2017

@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios
Contributor

Naios commented Oct 10, 2017

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Oct 12, 2017

Member

@Naios now this PR has a conflict which prevents us from going ahead... (and sorry it took such a long time for us to catch up with this).

Member

hkaiser commented Oct 12, 2017

@Naios now this PR has a conflict which prevents us from going ahead... (and sorry it took such a long time for us to catch up with this).

Naios added some commits Aug 13, 2017

Implement an API for asynchronous pack traversal
* This API is similar to traverse_pack however,
  it allows to suspend the control flow at any time.
Inherit the visitor rather than embedding it as a class member
* Makes it possible to introduce a class in the
  hierarchy (future_data for instance).
Use boost::intrusive_ptr rather than std::shared_ptr
* Makes it possible to create a shared future state from
  the traversal frame on the heap directly.
* Makes it possible to access the visitor while the traversal
  is in progress or finished.
* Use this to retrieve the counter in unit tests
  from the visitor.

Naios added some commits Aug 14, 2017

Use the async pack traversal API for when_all
* Extends when_all for the ability to pass non future
  values through and to accept nested futures.
* Cleanup the implementation and move internal stuff
  to a shared header (future_transforms.hpp).
Use the async pack traversal API for dataflow
* Extends dataflow for the ability to
  accept nested futures.
* Cleanup the implementation
Remove the future invalidation from dataflow
* Isn't needed anymore since the whole pack of futures is moved
  in a temporary scope which gets destructed after the
  traversal was finished.
  Previously the arguments were moved out of the
  object dependent on the signature of the corresponding
  dataflow evaluation function.
* Add a unit test for this
* Ref 10d4604
@Naios

This comment has been minimized.

Show comment
Hide comment
@Naios

Naios Oct 13, 2017

Contributor

@hkaiser No problem, I corrected the remaining merge conflicts.

Contributor

Naios commented Oct 13, 2017

@hkaiser No problem, I corrected the remaining merge conflicts.

@hkaiser hkaiser merged commit 353e280 into STEllAR-GROUP:master Oct 13, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Oct 13, 2017

Member

@Naios Thanks a lot again for your work on this!

Member

hkaiser commented Oct 13, 2017

@Naios Thanks a lot again for your work on this!

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