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

Allow for multiple Team splits and team cloning #663

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

Conversation

devreal
Copy link
Member

@devreal devreal commented Jul 19, 2019

Make the team hierarchy a tree and allow for cloning teams, which is useful in multi-threaded environments.

Fixes #658

@codecov
Copy link

codecov bot commented Jul 19, 2019

Codecov Report

Merging #663 into development will increase coverage by 0.06%.
The diff coverage is 91.76%.

@@               Coverage Diff               @@
##           development     #663      +/-   ##
===============================================
+ Coverage         85.1%   85.17%   +0.06%     
===============================================
  Files              336      336              
  Lines            24927    24993      +66     
  Branches         11766    11824      +58     
===============================================
+ Hits             21215    21288      +73     
- Misses            3689     3690       +1     
+ Partials            23       15       -8
Impacted Files Coverage Δ
dash/include/dash/Mutex.h 71.42% <ø> (ø) ⬆️
dash/include/dash/pattern/SeqTilePattern.h 92.39% <ø> (+0.17%) ⬆️
dash/include/dash/pattern/CSRPattern.h 91.86% <ø> (ø) ⬆️
dash/include/dash/pattern/ShiftTilePattern1D.h 91.66% <ø> (ø) ⬆️
dash/include/dash/pattern/LoadBalancePattern.h 84.88% <ø> (ø) ⬆️
dash/include/dash/Cartesian.h 90.83% <ø> (-0.37%) ⬇️
dash/include/dash/Shared.h 94.66% <0%> (ø) ⬆️
dash/src/Mutex.cc 87.5% <0%> (ø) ⬆️
dash/include/dash/algorithm/Reduce.h 97.5% <0%> (ø) ⬆️
dash/include/dash/Team.h 94.91% <0%> (+6.77%) ⬆️
... and 17 more

@bertwesarg
Copy link
Member

As you disable all of the traversial API, than why do we have this _children member at all? I think most users can live without this member. AFAIK, MPI does not have a child concept at all for its communicators and windows.

@devreal
Copy link
Member Author

devreal commented Jul 19, 2019

Good question. It doesn't seem to be used anywhere in the examples or applications and I don't see an immediate use-case to iterate over levels in the Team tree. My guess is that since the localities are hierarchical the teams were meant to be as well but I'm not sure that that is useful...

@bertwesarg
Copy link
Member

There is one reason though, the deconstructor, which also removes all children. I'm fine with this as is.

@@ -611,7 +672,8 @@ class Team
mutable team_unit_t _myid = UNDEFINED_TEAM_UNIT_ID;
mutable size_t _size = 0;
Team * _parent = nullptr;
Team * _child = nullptr;
std::vector<Team*> _children;
std::mutex _mutex;
Copy link
Member

Choose a reason for hiding this comment

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

mutable?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be mutable once we have const Teams :D

Copy link
Member

Choose a reason for hiding this comment

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

I only referenced the _mutex (see the M&M rule here). Also, I think that this and the _children would be a prerequisite for having const Teams in the future.

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.

Cannot split a team multiple times
2 participants