Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to dash::Future #451

Merged
merged 17 commits into from
Feb 14, 2018
Merged

Improvements to dash::Future #451

merged 17 commits into from
Feb 14, 2018

Conversation

devreal
Copy link
Member

@devreal devreal commented Oct 17, 2017

This PR extends dash::Future to allow for a meaningful test() function and proper tear-down. The latter is required to avoid memory leaks if dash::copy_async uses handles and the user does not wait() on it.

Fixes #416
See also #447

Copy link
Member

@fmoessbauer fmoessbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your changes look good so far, however I have some concerns regarding the performance of multiple relatively small async copy calls. Are there already benchmark results comparing before and after? If not, we could easily use the DASH implementation of Cafbench therefor.

@devreal
Copy link
Member Author

devreal commented Nov 3, 2017

Are there already benchmark results comparing before and after? If not, we could easily use the DASH implementation of Cafbench therefor.

I don't think we have a proper benchmark for it but it would be easy to create one. I might give it a try next week.

However, I don't immediately see how we could improve performance without sacrificing features. The current implementation of dash::Future is very basic and fails a simple use-case like while (fut.test()) do_something_else();, not to mention the potential for memory leaks.

I am open to ideas on how to save the dynamic allocation of the std::vector holding the handles (which probably is the critical addition in this PR).

@fuchsto
Copy link
Member

fuchsto commented Nov 9, 2017

Did not have a look at the implementation yet but the PR is a good catch.
More important than benchmarks for now: It's a new feature -> add unit tests!

@devreal
Copy link
Member Author

devreal commented Nov 14, 2017

@fuchsto Good point. Did that, made me change the implementation even more:

  1. Made dash::Future non-copyable, similar to std::future. We might want to add dash::SharedFuture at some point but that's a different story.
  2. Consolidated dash::internal::copy_impl and dash::internal::copy_async_impl into a single implementation to i) reduce duplicated code and ii) overlap the transfer to/from multiple remote units. There is still a metric ton of duplicated code in Copy.h but I didn't bother touching that as I expect it to change with [WIP] Fixing and improving algorithms with view expressions #410 anyway.
  3. Added dart_testall and dart_test, which upon success also ensure remote completion if required.
  4. All copy implementations now use handles: dash::copy waits for their (remote) completion while dash::copy_async puts them into a future.
  5. Handling of large transfers is now left to DART (see Chunk up puts/gets to cover large transfers #434), slightly simplifying the code in copy_impl.

Copy link
Member

@fmoessbauer fmoessbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good and IMO we can merge it. As the implementation of copy will change anyways I have no problem with using handles there. In the future we can improve that by providing optimized copy for tiny transfers.

The changes related to dash::Future are really good ones.

@devreal devreal merged commit 8b2d4b3 into development Feb 14, 2018
@fmoessbauer fmoessbauer deleted the feat-dart-handlefree branch February 14, 2018 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants