1983 Runtime optimizations 2 -- objgroup dispatch/locman/contexts #1984

lifflander · 2022-09-28T05:38:29Z

Fixes #1983

github-actions · 2022-09-28T07:58:26Z

Pipelines results

PR tests (gcc-12, ubuntu, mpich)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (gcc-5, ubuntu, mpich)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (gcc-6, ubuntu, mpich)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-3.9, ubuntu, mpich)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (gcc-9, ubuntu, mpich, zoltan)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-10, ubuntu, mpich)

Build for a501e7d (2023-01-30 20:12:35 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-9, ubuntu, mpich)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (gcc-7, ubuntu, mpich, trace runtime, LB)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-5.0, ubuntu, mpich)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (gcc-10, ubuntu, openmpi, no LB)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-13, alpine, mpich)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (gcc-8, ubuntu, mpich, address sanitizer)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (nvidia cuda 11.0, ubuntu, mpich)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-12, ubuntu, mpich)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-11, ubuntu, mpich)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (nvidia cuda 10.1, ubuntu, mpich)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (intel icpx, ubuntu, mpich)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-13, ubuntu, mpich)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (clang-14, ubuntu, mpich)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (gcc-11, ubuntu, mpich, json schema test)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

PR tests (intel icpc, ubuntu, mpich)

Build for 5b48207 (2022-11-16 20:54:24 UTC)

Compilation - successful

Testing - passed

Build log

src/vt/topos/location/location.impl.h

PhilMiller · 2022-09-28T17:46:04Z

src/vt/topos/location/location.impl.h

+) {
+  runnable::makeRunnable(msg, true, msg->getHandler(), theContext()->getNode())
+    .withTDEpochFromMsg()
+    .run();


Should this be .run() or .enqueue()?

This is a difficult one. .run() is correct for all the use cases that currently exist. It runs a system handler on the collection manager that generates the next runnable that is enqueued.

That seems kinda wasteful. Could we add a method in the collection manager to do that more directly?

That's what the next PR will do. It's already implemented on my other branch.

PhilMiller · 2022-09-28T17:46:45Z

src/vt/topos/location/location.impl.h

+  if (local_registered_.find(id) == local_registered_.end()) {
+    return routeMsg<MessageT>(id,home_node,msg);
+  } else {
+    return routeMsgHandlerLocal(msg);


Can we somehow take advantage of having already done the lookup in local_registered_ in this case?

We know it's local so we create the runnable directly for it to execute. local_registered_ does not hold the pointer to the actual object (it does not manage objects), just an entry that corresponds to its existence.

src/vt/runnable/runnable.cc

src/vt/objgroup/dispatch/dispatch.impl.h

src/vt/runnable/make_runnable.h

codecov · 2022-09-28T19:26:44Z

Codecov Report

Merging #1984 (a501e7d) into develop (e2665f5) will decrease coverage by 0.01%.
The diff coverage is 84.66%.

@@             Coverage Diff             @@
##           develop    #1984      +/-   ##
===========================================
- Coverage    84.87%   84.86%   -0.01%     
===========================================
  Files          723      720       -3     
  Lines        25549    25632      +83     
===========================================
+ Hits         21685    21753      +68     
- Misses        3864     3879      +15

Impacted Files	Coverage Δ
src/vt/context/runnable_context/collection.h	`100.00% <ø> (ø)`
src/vt/objgroup/manager.h	`100.00% <ø> (ø)`
src/vt/objgroup/manager.impl.h	`97.87% <0.00%> (+5.27%)`	⬆️
src/vt/registry/auto/auto_registry_common.h	`93.93% <ø> (-0.18%)`	⬇️
src/vt/runnable/runnable.h	`100.00% <ø> (ø)`
src/vt/objgroup/manager.cc	`90.90% <50.00%> (+1.62%)`	⬆️
src/vt/objgroup/manager.static.h	`76.00% <67.56%> (-24.00%)`	⬇️
src/vt/runnable/runnable.cc	`88.57% <87.09%> (+9.55%)`	⬆️
src/vt/topos/location/location.impl.h	`89.13% <88.23%> (-0.84%)`	⬇️
...erialization/messaging/serialized_messenger.impl.h	`99.27% <95.23%> (-0.73%)`	⬇️
... and 16 more

src/vt/objgroup/dispatch/dispatch.impl.h

src/vt/messaging/active.cc

src/vt/objgroup/manager.static.h

PhilMiller · 2022-12-02T00:47:00Z

Could you squash down the small patch-up commits that correct mistakes in earlier commits in the PR into those earlier commits?

PhilMiller

Would be good to get at least one other review on this, since it's pretty big

nmm0 · 2023-01-25T18:16:12Z

Any chance this could be split up into separate optimizations/PRs with maybe a short description of what's going on in it? It's a bit difficult to follow what all of the changes are doing or how they are more efficient.

For example there are a few changes (the std:function optimization) that I could easily approve already but the changes to objGroup dispatches I'm still trying to understand.

lifflander · 2023-01-25T20:17:08Z

Any chance this could be split up into separate optimizations/PRs with maybe a short description of what's going on in it? It's a bit difficult to follow what all of the changes are doing or how they are more efficient.

For example there are a few changes (the std:function optimization) that I could easily approve already but the changes to objGroup dispatches I'm still trying to understand.

Basically, the objgroup dispatch wasn't going through RunnableNew the same way a collection goes through it causing extra overhead. The old dispatch was creating a runnable that didn't actually know about the object it was dispatching to, so the Runnable had to call back into the objgroup manager to get the object and actually run the member function. The new code gets the object and associates it with the RunnableNew like the collection manager does.

lifflander · 2023-01-25T20:21:51Z

src/vt/runnable/runnable.cc


-  if (not is_void) {
-    if (is_obj) {
-      task_ = [=] { objgroup::dispatchObjGroup(msg_, handler); };


This is the old code, where we had to create a std::function that calls back into the objgroup manager code (because the object isn't known here).

src/vt/topos/location/location.impl.h

nmm0

Just some minor suggestions -- about the auto thing it's more up to personal preference. Just one thing to flag is Phil's comment, you mentioned it changed but it didn't look like the diff changed.

src/vt/objgroup/manager.cc

src/vt/runnable/make_runnable.h

nmm0 · 2023-01-27T00:42:02Z

src/vt/runnable/runnable.h

@@ -334,7 +339,12 @@ struct RunnableNew {
 private:
  detail::Contexts contexts_;               /**< The contexts  */
  MsgSharedPtr<BaseMsgType> msg_ = nullptr; /**< The associated message */
-  ActionType task_ = nullptr;               /**< The runnable's task  */
+  void* obj_ = nullptr;                     /**< Object pointer */
+  union {


I don't think we should hold this up because of it, since C++17 is still WIP, but this would be much more elegant with C++17 variant and std::visit

lifflander linked an issue Sep 28, 2022 that may be closed by this pull request

Make location manager more efficient #1983

Closed

3 tasks

lifflander force-pushed the 1983-make-location-manager-more-efficient branch from d22a1c4 to f8d4a00 Compare September 28, 2022 15:27

PhilMiller reviewed Sep 28, 2022

View reviewed changes

src/vt/topos/location/location.impl.h Outdated Show resolved Hide resolved

PhilMiller reviewed Sep 28, 2022

View reviewed changes

src/vt/topos/location/location.impl.h Outdated Show resolved Hide resolved

PhilMiller reviewed Sep 28, 2022

View reviewed changes

src/vt/runnable/runnable.cc Show resolved Hide resolved

PhilMiller reviewed Sep 28, 2022

View reviewed changes

src/vt/objgroup/dispatch/dispatch.impl.h Outdated Show resolved Hide resolved

PhilMiller reviewed Sep 28, 2022

View reviewed changes

src/vt/runnable/make_runnable.h Outdated Show resolved Hide resolved

lifflander marked this pull request as ready for review September 28, 2022 18:46

lifflander force-pushed the 1983-make-location-manager-more-efficient branch from 305d00b to f8d4a00 Compare September 28, 2022 18:51

nmm0 mentioned this pull request Oct 11, 2022

Meeting Agenda [do not close] #925

Open

stmcgovern force-pushed the 1983-make-location-manager-more-efficient branch from 8d9f1e3 to 69221fb Compare October 18, 2022 16:11

PhilMiller reviewed Oct 18, 2022

View reviewed changes

src/vt/objgroup/dispatch/dispatch.impl.h Outdated Show resolved Hide resolved

PhilMiller reviewed Oct 18, 2022

View reviewed changes

src/vt/objgroup/dispatch/dispatch.impl.h Outdated Show resolved Hide resolved

PhilMiller reviewed Oct 18, 2022

View reviewed changes

src/vt/messaging/active.cc Show resolved Hide resolved

lifflander force-pushed the 1983-make-location-manager-more-efficient branch from 69221fb to 6e0814e Compare October 24, 2022 23:01

PhilMiller reviewed Nov 11, 2022

View reviewed changes

src/vt/objgroup/manager.static.h Outdated Show resolved Hide resolved

PhilMiller reviewed Nov 11, 2022

View reviewed changes

src/vt/objgroup/manager.static.h Show resolved Hide resolved

lifflander force-pushed the 1983-make-location-manager-more-efficient branch from 17ab108 to 425c4de Compare November 16, 2022 20:46

lifflander force-pushed the 1983-make-location-manager-more-efficient branch from 5b48207 to 79fd8d5 Compare December 1, 2022 01:33

This was referenced Dec 1, 2022

Clean up location manager in advance of optimizations #2030

Closed

2030: Cleanup location management code #2031

Merged

lifflander force-pushed the 1983-make-location-manager-more-efficient branch from 79fd8d5 to 5c9ad2b Compare December 7, 2022 02:17

lifflander mentioned this pull request Dec 8, 2022

2027: Create Runnable inside Objgroup's invoke #2039

Merged

lifflander force-pushed the 1983-make-location-manager-more-efficient branch 2 times, most recently from 87a52cc to 9fbe4b6 Compare January 24, 2023 00:22

lifflander changed the title ~~1983 make location manager more efficient~~ 1983 Runtime optimizations 2 -- objgroup dispatch/locman/contexts Jan 24, 2023

lifflander requested a review from PhilMiller January 24, 2023 00:24

PhilMiller approved these changes Jan 24, 2023

View reviewed changes

lifflander requested a review from nmm0 January 24, 2023 19:52

lifflander commented Jan 25, 2023

View reviewed changes

nlslatt reviewed Jan 25, 2023

View reviewed changes

src/vt/topos/location/location.impl.h Outdated Show resolved Hide resolved

nlslatt reviewed Jan 25, 2023

View reviewed changes

src/vt/topos/location/location.impl.h Outdated Show resolved Hide resolved

nmm0 approved these changes Jan 27, 2023

View reviewed changes

lifflander force-pushed the 1983-make-location-manager-more-efficient branch from 60b7c95 to 0701e75 Compare January 30, 2023 20:09

lifflander added 8 commits January 30, 2023 12:12

#1983: runnable: overhaul to reduce allocations

bb9cb44

#1983: runnable: optimize objgroup dispatch

5cbaab9

#1983: location: refactor to reduce PendingSend allocation

8abf7e5

#1983: vrt_coll: do not capture on collection contexts

3a8ab34

#1983: runnable: adjust begin/end to start/finish

41779c2

#1983: location: remove duplicated code

362d702

#1983: location: fix typo

b1b853d

#1983: runnable: move lambda

a501e7d

lifflander force-pushed the 1983-make-location-manager-more-efficient branch from 0701e75 to a501e7d Compare January 30, 2023 20:12

lifflander merged commit b0894cf into develop Jan 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1983 Runtime optimizations 2 -- objgroup dispatch/locman/contexts #1984

1983 Runtime optimizations 2 -- objgroup dispatch/locman/contexts #1984

lifflander commented Sep 28, 2022

github-actions bot commented Sep 28, 2022 •

edited

Loading

PhilMiller Sep 28, 2022

lifflander Sep 28, 2022

PhilMiller Oct 18, 2022

lifflander Nov 17, 2022

PhilMiller Sep 28, 2022

lifflander Sep 28, 2022

codecov bot commented Sep 28, 2022 •

edited

Loading

PhilMiller commented Dec 2, 2022

PhilMiller left a comment

nmm0 commented Jan 25, 2023

lifflander commented Jan 25, 2023 •

edited

Loading

lifflander Jan 25, 2023

nmm0 left a comment

nmm0 Jan 27, 2023

1983 Runtime optimizations 2 -- objgroup dispatch/locman/contexts #1984

1983 Runtime optimizations 2 -- objgroup dispatch/locman/contexts #1984

Conversation

lifflander commented Sep 28, 2022

github-actions bot commented Sep 28, 2022 • edited Loading

Pipelines results

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

codecov bot commented Sep 28, 2022 • edited Loading

Codecov Report

PhilMiller commented Dec 2, 2022

PhilMiller left a comment

Choose a reason for hiding this comment

nmm0 commented Jan 25, 2023

lifflander commented Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

nmm0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 28, 2022 •

edited

Loading

codecov bot commented Sep 28, 2022 •

edited

Loading

lifflander commented Jan 25, 2023 •

edited

Loading