Fix Copy of C Array to Dash Array across multiple units

[rkowalewski]

Addresses #346

[rkowalewski]

It was not intended to submit the clang-format in this pull request. But now we have one.

[rkowalewski]

@fuchsto My changes assume that local memory is always contiguous. So it will probably not work with patterns where local memory is fragmented. Please consider for review this special case. I am not sure if any of our copy algorithms can handle this.

[devreal]

Comments inline.

[devreal]

Dangerous! dash::default_size_t may be 32 bit wide while num_elements is of type size_t.

[rkowalewski]

std::distance cannot return size_t because it may be negative. And then I check explicitly for a non-negative number.
Apart from that I would expect that dash::default_size_t is the same as size_t if available on the underlying platform. Is there any argument against this?

[devreal]

You are right, it is ssize_t. The case I had in mind is the following: the user might decides to compile with the flag DASH_ENABLE_DEFAULT_INDEX_TYPE_INT (for whatever reason) and then copy more than 4GB into the array, which causes num_elements to be truncated into nremaining. He might even pass size_t as template parameter IndexType to the global data structure so it's a valid operation. I think using the pattern's index-type is the safest bet. But that is certainly a highly pathologic case :)

[devreal]

See above.

[devreal]

Why not use dart_put and a final dart_flush after the loop?

[rkowalewski]

Of course you are right, thanks. I will do this. Did not think too much about this, I simply extended the already existing code.

[rkowalewski]

@devreal I have considered your comment and modified it appropriately.

After having a more detailed look into it I noticed that copy_async was not modified according to the changes in the blocking variant. The approach is now a bit more elegant in my opinion. dash::copy on local ranges is now dash::copy_async on local ranges followed by an immediate wait on the returned future.

As a consequence we do temporarily not support dash::GlobPtr as input to dash::copy because we need the pattern information in GlobIter. This was a discussion in #246. However it is supported again after merging #345 which enables pointer arithmetics on dash::GlobPtr across units.

[rkowalewski]

std::distance cannot return size_t because it may be negative. And then I check explicitly for a non-negative number.
Apart from that I would expect that dash::default_size_t is the same as size_t if available on the underlying platform. Is there any argument against this?

[rkowalewski]

Of course you are right, thanks. I will do this. Did not think too much about this, I simply extended the already existing code.

[fuchsto]

Support for strided / chunked local ranges has been requested in issue #116.

[fuchsto]

Ah, now I see your changes.

No, the implementation of dash::copy, especially in this respect, is not by accident. Isn't it exactly these kind of cheesy shortcuts we hate about some MPI implementations?

Some time ago I gave a talk on a DASH project meeting on dash::copy and why the blocking variant is not implemented as dash::async_copy + flush. The overhead is rather significant. The non-blocking variant creates dash::Future objects with lambdas, request handles are stored in a std::vector which leads to additional heap allocation, etc.
The non-blocking dash::copy_async (soon to be provided via launch policy) is only beneficial for reasonably high work loads for latency hiding. The dash::copy function interface has a dedicated benchmark suite that has been used to evaluate these tradeoffs.

In addition, the blocking algorithms in DASH should rely on specialized blocking operations provided by DART because these again can exploit the communication routines for this specific purpose - even if these are currently nearly identical.