1941 move EpochStack to TerminationDetector #1961
Conversation
Pipelines resultsPR tests (clang-5.0, ubuntu, mpich) Build for 6658d17 PR tests (clang-3.9, ubuntu, mpich) Build for 6658d17 PR tests (gcc-5, ubuntu, mpich) Build for 6658d17 PR tests (gcc-10, ubuntu, openmpi, no LB) Build for 6658d17 PR tests (gcc-6, ubuntu, mpich) Build for 6658d17 PR tests (clang-9, ubuntu, mpich) Build for 6658d17 PR tests (gcc-9, ubuntu, mpich, zoltan) Build for 6658d17 PR tests (nvidia cuda 11.0, ubuntu, mpich) Build for 6658d17 PR tests (clang-13, alpine, mpich) Build for 6658d17 PR tests (nvidia cuda 10.1, ubuntu, mpich) Build for 6658d17 PR tests (gcc-8, ubuntu, mpich, address sanitizer) Build for 6658d17 PR tests (intel icpx, ubuntu, mpich) Build for 6658d17 PR tests (clang-11, ubuntu, mpich) Build for 6658d17 PR tests (clang-12, ubuntu, mpich) Build for 6658d17 PR tests (clang-14, ubuntu, mpich) Build for 6658d17 PR tests (gcc-11, ubuntu, mpich) Build for bbb3077 PR tests (clang-10, ubuntu, mpich) Build for 6658d17 PR tests (clang-13, ubuntu, mpich) Build for 6658d17 PR tests (gcc-12, ubuntu, mpich) Build for 6658d17 PR tests (intel icpc, ubuntu, mpich) Build for 6658d17 PR tests (gcc-11, ubuntu, mpich, json schema test) Build for 6658d17 PR tests (gcc-7, ubuntu, mpich, trace runtime, LB) Build for 6658d17 |
Codecov Report
@@ Coverage Diff @@
## develop #1961 +/- ##
===========================================
- Coverage 84.11% 84.02% -0.09%
===========================================
Files 728 728
Lines 25604 25605 +1
===========================================
- Hits 21536 21514 -22
- Misses 4068 4091 +23
|
stmcgovern
force-pushed
the
1941-move-epoch-stack-to-terminationdetector
branch
2 times, most recently
from
7c36300 to
79e7faa
Compare
| auto const& non_zero = epoch_stack_.size() > 0; | ||
| vtAssertExprInfo( | ||
| non_zero and (epoch_stack_.top() == epoch.get() or epoch == no_epoch), | ||
| epoch, non_zero, epoch_stack_.top() |
There was a problem hiding this comment.
| epoch, non_zero, epoch_stack_.top() | |
| epoch, non_zero, non_zero ? epoch_stack_.top() : no_epoch |
| unsigned int size() const { return cur_; } | ||
|
|
||
| int cur_ = 0; | ||
| std::array<DataType, 64> stack_; |
There was a problem hiding this comment.
We'd discussed making this a potentially dynamically sized structure.
As in the scheduler queue, where we use a circular buffer, we may want to do something clever where we keep the std::array for the shallow case, and do a bulk hand-off to a growable structure for the bottom of the stack that's not changing as rapidly
There was a problem hiding this comment.
I'd suggest opening an issue and doing that in a follow-up commit
stmcgovern
force-pushed
the
1941-move-epoch-stack-to-terminationdetector
branch
from
79e7faa to
ddd37f8
Compare
| } | ||
|
|
||
| inline EpochType ActiveMessenger::getEpoch() const { | ||
| return getGlobalEpoch(); | ||
| return theTerm()->getEpoch(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't really like keeping these wrappers here, but I guess it spares us rewriting all the call sites at the same time. It does also allow for caching theTerm() in ActiveMessenger for internal usage
There was a problem hiding this comment.
I've not changed this. Do you think it's worth an issue to remove them or to go with the caching, @PhilMiller @lifflander ?
There was a problem hiding this comment.
Let's leave it be for now, and think about caching if that lookup ever turns up as a hot spot in profiles
|
Merging vs fixing stuff is up to you, @lifflander |
|
Since these problems already existed, I would prefer to merge and then fix them in a separate issue. |
…terminationdetector 1941 move EpochStack to TerminationDetector
Closes #1941. Add EpockStack to TerminationDetector.