875 Implement phase manager component #1123

lifflander · 2020-10-20T22:01:18Z

Fixes #875

Implement the new PhaseManager component
Remove a ton of code for the old, complex management of phases
Adapt examples and tests to new interface
Write documentation on new interface

codecov · 2020-10-20T22:41:35Z

Codecov Report

Merging #1123 into develop will decrease coverage by 0.11%.
The diff coverage is 88.46%.

@@             Coverage Diff             @@
##           develop    #1123      +/-   ##
===========================================
- Coverage    76.03%   75.92%   -0.12%     
===========================================
  Files          712      713       +1     
  Lines        27101    26940     -161     
===========================================
- Hits         20606    20453     -153     
+ Misses        6495     6487       -8

Impacted Files	Coverage Δ
src/vt/configs/arguments/app_config.h	`100.00% <ø> (ø)`
src/vt/configs/debug/debug_config.h	`85.71% <ø> (ø)`
src/vt/rdmahandle/manager.collection.impl.h	`0.00% <0.00%> (ø)`
src/vt/runtime/runtime.h	`100.00% <ø> (ø)`
src/vt/utils/memory/memory_usage.h	`90.90% <ø> (ø)`
src/vt/vrt/collection/balance/elm_stats.h	`100.00% <ø> (ø)`
...c/vt/vrt/collection/balance/lb_invoke/lb_manager.h	`100.00% <ø> (ø)`
src/vt/vrt/collection/balance/node_stats.cc	`71.02% <ø> (-3.71%)`	⬇️
src/vt/vrt/collection/balance/node_stats.h	`100.00% <ø> (ø)`
src/vt/vrt/collection/holders/base_holder.h	`100.00% <ø> (ø)`
... and 67 more

… usage

PhilMiller · 2020-10-23T03:56:02Z

src/vt/trace/trace.cc

@@ -148,6 +149,15 @@ void Trace::startup() /*override*/ {
  theSched()->registerTrigger(
    sched::SchedulerEvent::EndIdle, [this]{ endIdle(); }
  );
+
+  thePhase()->registerHookRooted(phase::PhaseHook::End, []{
+    auto const phase = thePhase()->getCurrentPhase();


Perhaps the hook interface should pass the phase as an argument? It could be overloaded to support hook that take it, or take nothing.

I like the simplicity of just an ActionType since it's so easy to call getCurrentPhase

PhilMiller · 2020-10-23T03:58:09Z

src/vt/trace/trace.cc

+  });
+
+  thePhase()->registerHookCollective(phase::PhaseHook::EndPostMigration, []{
+    theTrace()->flushTracesFile(false);


In the above vein, I'm doubtful we should actually be flushing every phase.

This doesn't actually flush unless the logic in the trace is activated. See flushTracesFile. It checks the size and some other conditions.

Now, I do agree it might be wasteful to spend a collective epoch on something that might do nothing every phase. But, they are pretty cheap anyway.

PhilMiller · 2020-10-23T04:04:37Z

src/vt/vrt/collection/manager.cc

+    }
+  });
+
+  // Second hook, select and then potentially start the LB


Are we defining the semantics that hooks for a given occasion run in the order they were inserted?

Yes, see the comments in the PhaseManager. registerHookCollective is collective and guarantees that the hooks are executed in the order in which they are registered.

PhilMiller · 2020-10-23T04:07:53Z

src/vt/vrt/collection/manager.impl.h

@@ -1979,6 +1980,21 @@ template <typename ColT, typename... Args>
   *  messages can be forwarded properly
   */
  theLocMan()->getCollectionLM<ColT,IndexType>(proxy);
+
+  /**
+   * Type-erase some lambdas for doing the collective broadcast that collects up


Does this even need to be a type-erased lambda? Every collection derives indirectly from ElementsStats, and with the simplified logic that just calls addNodeStats, I don't think syncNextPhase still needs to be templated.

Right now, there's no universal holder for collection elements. You have to know IndexT to send a message to them even if that message targets something like Migratable. In the future, we can reduce those requirements as we de-template collection elements.

src/vt/phase/registered_phase_hook.h

PhilMiller · 2020-10-23T04:20:39Z

src/vt/vrt/collection/manager.cc

+  thePhase()->registerHookCollective(phase::PhaseHook::End, []{
+    auto const cur_phase = thePhase()->getCurrentPhase();
+    theLBManager()->selectStartLB(cur_phase);
+  });


In reviewing LBManager, it threw me off a bit to see the function defined there, but not registered there. I understand why that is, but it's a bit disorienting to have it here nonetheless.

If we want to be fancy, we could perhaps give names to the various hooks, define dependencies between them, and rely on the phase manager to run them in a suitable order based on a topological sort, with synchronization between.

If we had that, would that also obviate the distinction between PhaseHook::End and PhaseHook::EndPostMigration?

src/vt/phase/phase_manager.h

PhilMiller · 2020-10-23T04:29:49Z

src/vt/phase/phase_manager.cc

+  // Start with a reduction to sure all nodes are ready for this
+  auto cb = theCB()->makeBcast<PhaseManager, NextMsg, &PhaseManager::nextPhaseReduce>(proxy);
+  auto msg = makeMessage<NextMsg>();
+  proxy.reduce(msg.get(), cb);
+
+  theSched()->runSchedulerWhile([this]{ return not reduce_next_phase_done_; });
+  reduce_next_phase_done_ = false;


This is basically just a barrier() on a private collective scope. Do we want to just add that operation, rather than open-coding it here (and possibly duplicated elsewhere)?

No, because the semantic would be different. Barriers generally have global ordering to them and are shared across components. The reduce used here is scoped within the PhaseManager due to reduce scoping rules per component in VT (each component has its own scope by default).

I think you didn't mean use theCollective()->barrier(), you meant to refactor this as a pattern templated on an objgroup proxy (or maybe any proxy). That seems reasonable for follow-on work.

Right, it's refactoring the scoped "reduce to a (logical) no-op handler and spin on that" into an objgroup proxy method. Want to open that issue and resolve this comment?

src/vt/phase/phase_manager.cc

PhilMiller · 2020-10-23T04:34:00Z

This all looks really good. Makes the runtime more powerful and flexible, and cuts 400 lines of code. Typos and naming aside, nothing I noted should really block merging this. I do think making ordering more explicit, rather than the implicit "in order of registration", would be of benefit though.

cz4rs

looks good, nothing more to add than what Phil wrote

src/vt/phase/phase_manager.cc

nlslatt

Looks good to me

lifflander · 2020-10-23T21:00:40Z

Here is an overview of what got changed by this pull request:

Clones added
============
- examples/collection/lb_iter.cc  1
- tests/unit/collection/test_lb.extended.cc  2
- tests/unit/collection/test_lb_lite.extended.cc  1

See the complete overview on Codacy

lifflander added 2 commits October 20, 2020 14:48

#875: phase: start with a very simple PhaseManager

8ab5b0e

#875: phase: add cc file, promote to objgroup

be10a08

lifflander added 15 commits October 20, 2020 16:14

#875: phase: implement hooks, (de-)registration

b7ed578

#875: phase: add reduction for nextPhaseCollective

a783b6f

#875: phase: make hooks collective/rooted

77c6703

#875: phase: add call to invoke the hooks

3289ed9

#875: runtime: change deps for startup order

1b18e41

#875: utils: make memory usage component use hook for printing memory…

fe077e8

… usage

#875: phase: add initial time start hooks are invoked

11714f3

#875: trace: add trace hooks for flushing and toggling

66d8270

#875: phase: masssive simplification of phase code

3d0fda4

#875: example: conform lb_iter to new interface

7b46344

#875: phase: add new debug print category

2c33fec

#875: phase: get rid of old, unneeded phase messages

e78c6de

#875: phase: fix typo in debug print statement

77f79f7

#875: test: massively simplify test with new phase management

35be795

#875: test: conform all tests to use new interface

0f6f0e8

lifflander marked this pull request as ready for review October 22, 2020 05:57

lifflander requested review from PhilMiller, cz4rs, JacobDomagala, jstrzebonski, bradybray, nlslatt and pnstickne October 22, 2020 05:57

lifflander added 2 commits October 22, 2020 00:51

#875: test: fix one last test

c38889f

#875: phase: remove listeners, add RDMA hook for collections

01e8bb7

lifflander requested a review from nmm0 October 22, 2020 18:39

#875: test: fix test using a hook for trigger so stats exists

a2f3af3

lifflander added 5 commits October 22, 2020 15:59

#875: docs: fix another typo

4e22bfb

#875: test: combine into one condition

3c516f5

#875: phase: add manual run for hooks for testing purposes

2c92f94

#875: test: adapt rdma test to run hooks the new way

d975596

#875: test: add new test for phase management

523b678

PhilMiller reviewed Oct 23, 2020

View reviewed changes

src/vt/phase/registered_phase_hook.h Outdated Show resolved Hide resolved

PhilMiller reviewed Oct 23, 2020

View reviewed changes

src/vt/phase/phase_manager.h Outdated Show resolved Hide resolved

PhilMiller reviewed Oct 23, 2020

View reviewed changes

src/vt/phase/phase_manager.h Outdated Show resolved Hide resolved

PhilMiller reviewed Oct 23, 2020

View reviewed changes

src/vt/phase/phase_manager.cc Show resolved Hide resolved

PhilMiller mentioned this pull request Oct 23, 2020

LB hangs with multiple live collections #1055

Closed

cz4rs approved these changes Oct 23, 2020

View reviewed changes

lifflander added 3 commits October 23, 2020 12:24

#875: phase: change name of file to phase_hook_id

dad9515

#875: phase: fix typos

cc64a8b

#875: phase: remove some extra conditions not needed

6f457f2

lifflander mentioned this pull request Oct 23, 2020

Explore expanding PhaseManager to have explicit dependencies across hooks #1124

Open

PhilMiller reviewed Oct 23, 2020

View reviewed changes

src/vt/phase/phase_manager.cc Outdated Show resolved Hide resolved

PhilMiller approved these changes Oct 23, 2020

View reviewed changes

#875: phase: change assert to abort

620d4b0

nlslatt approved these changes Oct 23, 2020

View reviewed changes

#875: phase: add macro with date for phase manager feature

9ee2b0b

lifflander merged commit 01ccf10 into develop Oct 23, 2020

cz4rs mentioned this pull request Jun 15, 2021

move vt version number into a separate file #1467

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

875 Implement phase manager component #1123

875 Implement phase manager component #1123

lifflander commented Oct 20, 2020 •

edited

Loading

codecov bot commented Oct 20, 2020 •

edited

Loading

PhilMiller Oct 23, 2020

lifflander Oct 23, 2020

PhilMiller Oct 23, 2020

lifflander Oct 23, 2020

PhilMiller Oct 23, 2020

lifflander Oct 23, 2020

PhilMiller Oct 23, 2020

lifflander Oct 23, 2020

PhilMiller Oct 23, 2020

PhilMiller Oct 23, 2020

PhilMiller Oct 23, 2020

lifflander Oct 23, 2020

lifflander Oct 23, 2020

PhilMiller Oct 23, 2020

PhilMiller commented Oct 23, 2020

cz4rs left a comment

nlslatt left a comment

lifflander commented Oct 23, 2020

875 Implement phase manager component #1123

875 Implement phase manager component #1123

Conversation

lifflander commented Oct 20, 2020 • edited Loading

codecov bot commented Oct 20, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilMiller commented Oct 23, 2020

cz4rs left a comment

Choose a reason for hiding this comment

nlslatt left a comment

Choose a reason for hiding this comment

lifflander commented Oct 23, 2020

lifflander commented Oct 20, 2020 •

edited

Loading

codecov bot commented Oct 20, 2020 •

edited

Loading