1052 Implement epoch scopes #1053
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lifflander
force-pushed
the
1052-epoch-scopes
branch
from
87487f7 to
768a1ea
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1053 +/- ##
===========================================
- Coverage 81.67% 81.17% -0.50%
===========================================
Files 733 732 -1
Lines 27909 28281 +372
===========================================
+ Hits 22795 22958 +163
- Misses 5114 5323 +209
|
PhilMiller
reviewed
Sep 22, 2020
PhilMiller
reviewed
Sep 22, 2020
src/vt/epoch/epoch_manip_make.cc
Outdated
|
|
||
| EpochType EpochManip::nextSeq(EpochScopeType scope, bool is_collective) { | ||
| auto& scope_map = is_collective ? scope_collective_ : scope_rooted_; | ||
| if (scope_map.find(scope) == scope_map.end()) { |
There was a problem hiding this comment.
Could we simplify the logic here by having an invariant that all create scopes exist in scope_map, rather than seemingly inserting them on first use?
There was a problem hiding this comment.
There's a race if one rank creates a scope before the others, then a message gets sent which creates a child epoch (think rooted epoch within that scope) from that one. It will call nextSeq to generate one.
There was a problem hiding this comment.
Please write that up in a comment here, and reference it explicitly in scope creation.
There was a problem hiding this comment.
I think there may still be a race in this, but it would be I think bad application design that would let it happen
PhilMiller
reviewed
Sep 22, 2020
src/vt/epoch/epoch_manip_make.cc
Outdated
Comment on lines
171
to
173
| vtAssert(next >= 0, "Scope must be greater than 0"); | ||
| vtAssert(next < scope_limit, "Scope must be less than the scope limit"); | ||
| vtAssert(not live_scopes_.exists(next), "Scope must not already exist"); |
There was a problem hiding this comment.
These should probably be vtAbortIf instead - the counter is an exhaustible resource, and we have no reason to expect it to only exhaust in debugging builds but not release.
There was a problem hiding this comment.
Ok, I've changed one above these to vtAbortIf (which is the out-of-space error), the others are just a logic error. Should those be changed also?
|
@PhilMiller I think I'm missing some major code to carry to scope down to child epochs created. |
PhilMiller
reviewed
Sep 22, 2020
PhilMiller
reviewed
Sep 22, 2020
PhilMiller
reviewed
Sep 22, 2020
PhilMiller
reviewed
Sep 22, 2020
PhilMiller
reviewed
Sep 22, 2020
lifflander
force-pushed
the
1052-epoch-scopes
branch
from
768a1ea to
c8d8df9
Compare
PhilMiller
reviewed
Sep 22, 2020
PhilMiller
reviewed
Sep 23, 2020
lifflander
force-pushed
the
1052-epoch-scopes
branch
from
c0a1656 to
40ebed6
Compare
|
I'm thinking about adding a |
|
So I've found a major problem in how collective epoch scopes are implement wrt nesting. Here's a fairly simple example program that hangs most of the time. The while loop in the scheduler is needed to perturb the order and increase the chance that a message arrives in between. using TestMsg = vt::Message;
static void nestedHandler(TestMsg* msg);
static void nestedHandler2(TestMsg* msg);
TEST_F(TestEpochScopes, test_epoch_scope_3) {
// Test creating nested collective epochs
auto const this_node = theContext()->getNode();
auto const num_nodes = theContext()->getNumNodes();
// Must have at least 2 nodes to run this test
if (num_nodes < 2) {
return;
}
auto scope = theEpoch()->makeScopeCollective();
for (int i = 0; i < 100; i++) {
auto ep = scope.makeEpochCollective();
theMsg()->pushEpoch(ep);
auto next_node = (this_node + 1) % num_nodes;
auto msg = makeMessage<TestMsg>();
theMsg()->sendMsg<TestMsg, nestedHandler>(next_node, msg);
for (int j = 0; j < 5; j++) {
vt::runScheduler();
}
theMsg()->popEpoch(ep);
theTerm()->finishedEpoch(ep);
}
}
static void nestedHandler(TestMsg* msg) {
auto ep = theTerm()->makeEpochCollective();
theMsg()->pushEpoch(ep);
auto const this_node = theContext()->getNode();
auto const num_nodes = theContext()->getNumNodes();
auto next_node = (this_node + 1) % num_nodes;
auto nmsg = makeMessage<TestMsg>();
theMsg()->sendMsg<TestMsg, nestedHandler2>(next_node, nmsg);
theMsg()->popEpoch(ep);
theTerm()->finishedEpoch(ep);
}
static void nestedHandler2(TestMsg* msg) {} |
lifflander
force-pushed
the
1052-epoch-scopes
branch
from
88242c2 to
06ec8fe
Compare
|
Clones added
============
- tests/unit/epoch/test_epoch_scopes.cc 2
See the complete overview on Codacy |
This was referenced Oct 14, 2020
lifflander
force-pushed
the
1052-epoch-scopes
branch
from
4f252a4 to
07c1580
Compare
lifflander
force-pushed
the
1052-epoch-scopes
branch
2 times, most recently
from
0d5ca12 to
57aa348
Compare
|
PR tests (clang-8, alpine, mpich) |
|
PR tests (intel 19, ubuntu, mpich) Build for 94bfdd6 |
|
PR tests (nvidia cuda 11.0, ubuntu, mpich) Build for 94bfdd6 |
|
PR tests (nvidia cuda 10.1, ubuntu, mpich) Build for 94bfdd6 |
lifflander
force-pushed
the
1052-epoch-scopes
branch
from
165f8c1 to
94bfdd6
Compare
|
PR tests (gcc-5, ubuntu, mpich) Build for 94bfdd6 |
|
PR tests (clang-5.0, ubuntu, mpich) Build for 94bfdd6 |
|
PR tests (gcc-9, ubuntu, mpich, zoltan) Build for 94bfdd6 |
|
PR tests (clang-3.9, ubuntu, mpich) Build for 94bfdd6 |
|
PR tests (gcc-10, ubuntu, openmpi, no LB) Build for 94bfdd6 |
|
PR tests (gcc-6, ubuntu, mpich) Build for 94bfdd6 |
|
PR tests (gcc-8, ubuntu, mpich, address sanitizer) Build for 94bfdd6 |
|
PR tests (gcc-7, ubuntu, mpich, trace runtime, LB) Build for 94bfdd6 |
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1052
TODO:
Addresses multiple issues: