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

Implement global-to-global dash::copy based on #659 #660

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

bertwesarg
Copy link
Member

No description provided.

@bertwesarg bertwesarg requested a review from devreal July 14, 2019 19:58
class GlobInputIt,
class GlobOutputIt,
typename ValueType = typename GlobInputIt::value_type>
GlobOutputIt copy_to(
Copy link
Member

Choose a reason for hiding this comment

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

Why not require the team of units involved in the global-to-global copy to be passed and identify the direction based on that?

Copy link
Member Author

@bertwesarg bertwesarg Jul 15, 2019

Choose a reason for hiding this comment

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

That would result in an API change for dash::copy, right? Is this intended? It also will result in a runtime decision. But at least we could stick with dash::copy instead of two new names. Not my decision.

Btw, should global-to-global also get an async variant?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to have a selection based on an template parameter, either true/false or an enum. That would retain the API and we have compile time selection.

Copy link
Member

Choose a reason for hiding this comment

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

You're right, the copy has to be collective over one of the two teams (either from the input or from the output iterators). There should be no default IMO because that may only be valid in 50% of the cases. A template parameter is fine with me, preferably an enum (or struct encapsulating the decision on what is local and what is remote).

Another note: global-to-global needs to have a barrier at the end to make sure that all data has been exchanged (including transfers into the unit's local memory).

Btw, should global-to-global also get an async variant?

I'd say yes. It would require a non-blocking barrier to facilitate the test/wait in the future though, something we don't have in DART atm (but which is easily added for dart-mpi, not sure about dart-gaspi. @dhinf ?)

Copy link
Member

Choose a reason for hiding this comment

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

Let's put it this way. GASPI doesn't provide an IBarrier :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Another note: global-to-global needs to have a barrier at the end to make sure that all data has been exchanged (including transfers into the unit's local memory).

They are already there and also on the function entry. What is the policy here in general?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd say yes. It would require a non-blocking barrier to facilitate the test/wait in the future though, something we don't have in DART atm (but which is easily added for dart-mpi, not sure about dart-gaspi. @dhinf ?)

I think we should postpone this then.

@bertwesarg bertwesarg changed the base branch from bug-656-copy to development March 3, 2020 12:33
This way one can call copy_impl multiple times, before triggering local
copies.
Active team selection is now done by tag struct argument.
@codecov
Copy link

codecov bot commented Mar 3, 2020

Codecov Report

Merging #660 into development will increase coverage by 0.39%.
The diff coverage is 65.74%.

@@               Coverage Diff               @@
##           development     #660      +/-   ##
===============================================
+ Coverage        83.65%   84.04%   +0.39%     
===============================================
  Files              336      336              
  Lines            24968    25063      +95     
  Branches         11354    11416      +62     
===============================================
+ Hits             20887    21065     +178     
- Misses            3692     3694       +2     
+ Partials           389      304      -85     
Impacted Files Coverage Δ
dash/include/dash/algorithm/Copy.h 59.78% <45.45%> (-5.60%) ⬇️
dash/test/algorithm/CopyTest.cc 97.62% <97.36%> (-0.02%) ⬇️
dash/test/container/MatrixTest.cc 99.33% <100.00%> (+<0.01%) ⬆️
dash/include/dash/GlobRef.h 76.99% <0.00%> (-13.28%) ⬇️
dash/include/dash/Distribution.h 84.84% <0.00%> (-3.04%) ⬇️
dash/include/dash/util/UnitLocality.h 56.52% <0.00%> (-1.45%) ⬇️
dash/include/dash/memory/GlobHeapMem.h 96.47% <0.00%> (-0.71%) ⬇️
dash/include/dash/pattern/BlockPattern.h 96.31% <0.00%> (-0.57%) ⬇️
dash/include/dash/Cartesian.h 89.16% <0.00%> (+0.83%) ⬆️
dash/include/dash/Array.h 88.94% <0.00%> (+0.96%) ⬆️
... and 22 more

@bertwesarg
Copy link
Member Author

This is green now (though I'm note sure what this MatrixTest tried to achieved). Any more comments? Thanks.

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.

3 participants