#962 Msg API - switch to implicit conversion #969

pnstickne · 2020-08-04T07:30:41Z

Changes theMsg()'s sendMsg and broadcastMsg to perform an implicit std::move on the MsgPtr& argument.

Change all core usages to supply a MsgPtr& (and all but two usage in tests).

This change can make some code that worked before fail is the implicit move-like semantics are not honored.

Notes:

This currently uses an implicit proxy to unify both MsgPtr& and Msg*. However, this type can be eliminated if dropping Msg* directly and always accepting MsgPtr&.

Over all the current code in VT, the pattern of creating a message is "the method" so there is no need to accept a MsgPtr or MsgPtr&&.

The implicit type is still very hand for transitioning over.

codecov · 2020-08-04T08:41:43Z

Codecov Report

Merging #969 into develop will decrease coverage by 0.15%.
The diff coverage is 82.74%.

@@             Coverage Diff             @@
##           develop     #969      +/-   ##
===========================================
- Coverage    77.56%   77.40%   -0.16%     
===========================================
  Files          667      665       -2     
  Lines        25614    25522      -92     
===========================================
- Hits         19868    19756     -112     
- Misses        5746     5766      +20

Impacted Files	Coverage Δ
examples/rdma/rdma_simple_put.cc	`13.11% <0.00%> (ø)`
examples/rdma/rdma_simple_put_direct.cc	`19.04% <0.00%> (ø)`
...c/vt/collective/reduce/operators/default_op.impl.h	`100.00% <ø> (ø)`
src/vt/event/event.cc	`60.56% <0.00%> (+1.25%)`	⬆️
src/vt/group/group_info.impl.h	`37.70% <0.00%> (ø)`
src/vt/parameterization/parameterization.h	`100.00% <ø> (ø)`
...allback/proxy_bcast/callback_proxy_bcast_tl.impl.h	`56.00% <0.00%> (-2.34%)`	⬇️
src/vt/trace/trace_user_event.cc	`44.26% <0.00%> (ø)`
src/vt/vrt/collection/balance/node_stats.cc	`46.62% <0.00%> (-0.27%)`	⬇️
src/vt/vrt/collection/manager.cc	`68.29% <0.00%> (ø)`
... and 93 more

PhilMiller · 2020-08-04T16:59:40Z

Looks good so far. Looking forward to seeing all of the msg.get() in caller code disappear

src/vt/messaging/active.h

src/vt/registry/auto/auto_registry.h

pnstickne · 2020-08-14T08:05:40Z

@lifflander "Works Here", ahah ^_^

src/vt/collective/reduce/reduce.impl.h

PhilMiller · 2020-08-17T15:53:40Z

src/vt/messaging/message/smart_ptr.h

+ * \brief Helper to unify 'stealing' message ownership.
+ *
+ * This type is not intented to be used directly. It uses implicit conversion
+ * construtors to perform a 'std::move' on a \c MsgPtr<MsgT>&.


Typo 'constructors'

... It still reads 'construtors' (missing 'c'). Neglected push?

PhilMiller · 2020-08-17T16:04:24Z

src/vt/vrt/collection/balance/node_stats.cc

+  auto msg_hold = promoteMsg(msg.get());
+  theMsg()->broadcastMsg<MsgType,CollectionManager::releaseLBPhase>(msg);
+
+  CollectionManager::releaseLBPhase(msg_hold.get());


This repeated pattern of get, promote, broadcast, and deliver further favors making broadcast include local delivery.

@PhilMiller broadcastMsgIncludingSelf? A mouthful..

src/vt/termination/termination.cc

PhilMiller · 2020-08-17T16:30:00Z

This is good progress. I like that this makes promoteMsg an indicator that something weird is being done, and that the surrounding code needs more care and attention on it. Ideally, we'd be able to reduce the number of call sites accordingly, now that we can see them clearly.

src/vt/group/collective/group_info_collective.cc

PhilMiller

I'm generally pretty happy with this.

pnstickne · 2020-08-24T23:56:41Z

The failing test is addressed in #990. After #990 is merged the MsgPtrThief tests here should be updated to be no-MPI.

bradybray · 2020-08-30T20:38:27Z

Looks good save for the one failed build.

- This allows the same callsite to, eg., bind to either a Msg<T>* or a MsgPtr<T>& without requiring duplicated methods. However, it also requires that the templated forms accepting a 'FunctorT' as the first parameter specify the message type as such can no longer be inferred.

- Despite very few usages, it's also quite confusing auto-null a ptr. It also does not work towards elimination of using T* directly. The usages of T* are restricted to being const, which should effectively only be 'msg.get()' cases.

- Now issues a vtAssert in some cases. In particular, get() will never return null as this much more eagerly exposes when a MsgPtr& has been [unexpectedly] moved by the API.

- Examples / tutorials / tests.

- The new name should (hopefully) much more clearly represent what is occuring and/or should be expected. This doesn't steal a Msg*, only a MsgPtr. This is designed to be used to transitional API which still accepts a Msg* which can be targeted as [deprecated]. - Tests cover expected thieving ability.

- The msg thief tests cover the expected lifetimes of all allowed usages - MsgT, MgsPtr<MsgT>&, MsgPtr<MsgT>&&.

- All (immediate) test directories are added in CMake instead of being manually specified. This reduces the chance of misplacing and expected resource.. such as 'context'. - REMOVED the non-compiled / not-executed context tests. These tests are woefully out of date and fail to compile for multiple reasons. See #999.

- Clarify ownership, provide std::move example.

- Not needed, if ever might have been.

lifflander · 2020-09-08T18:21:41Z

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

Clones removed
==============
+ tests/unit/memory/test_memory_lifetime.cc  -4
+ tutorial/tutorial_1g.h  -3

See the complete overview on Codacy

pnstickne force-pushed the 962-msg-api-redux branch 2 times, most recently from 65eed88 to 461065a Compare August 4, 2020 08:00

lifflander reviewed Aug 5, 2020

View reviewed changes

src/vt/messaging/active.h Outdated Show resolved Hide resolved

lifflander reviewed Aug 5, 2020

View reviewed changes

src/vt/registry/auto/auto_registry.h Show resolved Hide resolved

pnstickne force-pushed the 962-msg-api-redux branch 4 times, most recently from b37a297 to 7b623c9 Compare August 14, 2020 08:05

PhilMiller reviewed Aug 17, 2020

View reviewed changes

src/vt/collective/reduce/reduce.impl.h Outdated Show resolved Hide resolved

PhilMiller reviewed Aug 17, 2020

View reviewed changes

src/vt/termination/termination.cc Outdated Show resolved Hide resolved

pnstickne force-pushed the 962-msg-api-redux branch from ec9669d to 0cb4044 Compare August 18, 2020 18:16

pnstickne marked this pull request as ready for review August 18, 2020 18:17

lifflander mentioned this pull request Aug 18, 2020

Meeting Agenda [do not close] #925

Open

PhilMiller reviewed Aug 18, 2020

View reviewed changes

src/vt/group/collective/group_info_collective.cc Outdated Show resolved Hide resolved

PhilMiller reviewed Aug 18, 2020

View reviewed changes

pnstickne force-pushed the 962-msg-api-redux branch 3 times, most recently from 25f5f03 to 72a7735 Compare August 23, 2020 23:32

pnstickne requested review from PhilMiller and lifflander August 24, 2020 01:27

pnstickne requested review from bradybray, fnrizzi and JacobDomagala August 25, 2020 16:46

bradybray approved these changes Aug 30, 2020

View reviewed changes

pnstickne force-pushed the 962-msg-api-redux branch from d116675 to ad39899 Compare September 1, 2020 07:10

PhilMiller approved these changes Sep 1, 2020

View reviewed changes

pnstickne and others added 11 commits September 8, 2020 11:18

#962 send- remove ability to take T*&

fa17027

- Despite very few usages, it's also quite confusing auto-null a ptr. It also does not work towards elimination of using T* directly. The usages of T* are restricted to being const, which should effectively only be 'msg.get()' cases.

#962 send- MsgPtr assertions increased

8efedc8

- Now issues a vtAssert in some cases. In particular, get() will never return null as this much more eagerly exposes when a MsgPtr& has been [unexpectedly] moved by the API.

#963 send- core uses MsgPtr& for theMsg() API

4c7b08d

#962 send- update non-core to use MsgPtr& API

e04350b

- Examples / tutorials / tests.

#962 tests/lifetime - remove duplicate tests

7774e99

- The msg thief tests cover the expected lifetimes of all allowed usages - MsgT, MgsPtr<MsgT>&, MsgPtr<MsgT>&&.

#962 message- add ownership transer in documentation

257300f

#962 examples- clarify ownership in examples

27b6875

- Clarify ownership, provide std::move example.

#962 registry- remove needless message-passing to auto-registry methods

89bf296

- Not needed, if ever might have been.

pnstickne force-pushed the 962-msg-api-redux branch from ad39899 to 89bf296 Compare September 8, 2020 18:19

lifflander approved these changes Sep 8, 2020

View reviewed changes

lifflander merged commit 6a80f05 into develop Sep 8, 2020

PhilMiller deleted the 962-msg-api-redux branch September 8, 2020 20:34

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

#962 Msg API - switch to implicit conversion #969

#962 Msg API - switch to implicit conversion #969

pnstickne commented Aug 4, 2020 •

edited

Loading

codecov bot commented Aug 4, 2020 •

edited

Loading

PhilMiller commented Aug 4, 2020

pnstickne commented Aug 14, 2020

PhilMiller Aug 17, 2020

pnstickne Aug 24, 2020

PhilMiller Aug 26, 2020

PhilMiller Aug 17, 2020

pnstickne Aug 23, 2020

PhilMiller commented Aug 17, 2020

PhilMiller left a comment

pnstickne commented Aug 24, 2020

bradybray commented Aug 30, 2020

lifflander commented Sep 8, 2020

#962 Msg API - switch to implicit conversion #969

#962 Msg API - switch to implicit conversion #969

Conversation

pnstickne commented Aug 4, 2020 • edited Loading

codecov bot commented Aug 4, 2020 • edited Loading

Codecov Report

PhilMiller commented Aug 4, 2020

pnstickne commented Aug 14, 2020

PhilMiller Aug 17, 2020

Choose a reason for hiding this comment

pnstickne Aug 24, 2020

Choose a reason for hiding this comment

PhilMiller Aug 26, 2020

Choose a reason for hiding this comment

PhilMiller Aug 17, 2020

Choose a reason for hiding this comment

pnstickne Aug 23, 2020

Choose a reason for hiding this comment

PhilMiller commented Aug 17, 2020

PhilMiller left a comment

Choose a reason for hiding this comment

pnstickne commented Aug 24, 2020

bradybray commented Aug 30, 2020

lifflander commented Sep 8, 2020

pnstickne commented Aug 4, 2020 •

edited

Loading

codecov bot commented Aug 4, 2020 •

edited

Loading