796 Reduction Scoping #832

lifflander · 2020-06-04T23:41:05Z

Fixes #796
Fixes #821

Rewrite machinery for scoping and stamping reductions.

This PR solves long-standing issues with collisions in asynchronous reductions. Create reducer instances with scoping for each instance and stamping reductions within a scope for identification across reductions in the same scope.

Note, this a breaking API change.

theCollective()->reduce<>(..) is no longer allowed.
You may access theCollective()->global()->reduce(..) but this not recommended.

Every component now has its own scope with an accessor in the base component class.
Every collection instance has its own reducer scope (accessed automatically through the proxy.reduce)
Every group has its own scope (tied to a group's default spanning tree)
Every obj group has its own scope (accessed automatically through proxy.reduce)

The new implementation is far more compact in bits sent with a reduction message by using SafeUnion. Probably 1/3 of the space required before.

codecov · 2020-06-05T00:24:15Z

Codecov Report

Merging #832 into develop will increase coverage by 0.09%.
The diff coverage is 86.82%.

@@             Coverage Diff             @@
##           develop     #832      +/-   ##
===========================================
+ Coverage    80.41%   80.50%   +0.09%     
===========================================
  Files          347      353       +6     
  Lines        10933    11205     +272     
===========================================
+ Hits          8792     9021     +229     
- Misses        2141     2184      +43

Impacted Files	Coverage Δ
src/vt/collective/reduce/scoping/strong_types.h	`0.00% <0.00%> (ø)`
src/vt/objgroup/proxy/proxy_objgroup.h	`100.00% <ø> (ø)`
src/vt/vrt/collection/reducable/reducable.h	`100.00% <ø> (ø)`
src/vt/objgroup/proxy/proxy_objgroup.impl.h	`43.90% <50.00%> (ø)`
src/vt/collective/reduce/reduce_manager.impl.h	`60.00% <60.00%> (ø)`
src/vt/collective/reduce/reduce_scope.impl.h	`64.28% <64.28%> (ø)`
src/vt/vrt/collection/manager.impl.h	`85.01% <75.75%> (-0.23%)`	⬇️
src/vt/utils/adt/union.h	`83.57% <83.57%> (ø)`
src/vt/collective/reduce/reduce_scope.h	`90.32% <90.32%> (ø)`
src/vt/collective/reduce/reduce.impl.h	`97.67% <97.56%> (+1.79%)`	⬆️
... and 16 more

PhilMiller · 2020-06-05T01:35:12Z

tests/unit/utils/test_safe_union.cc

+  vt::adt::SafeUnion<int64_t, char> y2;
+
+  // This makes an assertion about alignment that will hold across these two
+  // types


If you're concerned about alignment, you can (also) assert on alignof

tests/unit/utils/test_safe_union.cc

src/vt/utils/adt/union.h

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

PhilMiller · 2020-06-05T02:40:28Z

The Sizer giving the sum of sizes rather than the maximum is the only really substantial concern I have here. Address that, or be explicit that you're OK with it being like that, and I'm +1

PhilMiller

You've got my comments. The code is generally OK.

lifflander · 2020-06-09T01:11:25Z

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

Clones added
============
- src/vt/utils/adt/union.h  2
         

Clones removed
==============
+ src/vt/vrt/collection/reducable/reducable.impl.h  -4

See the complete overview on Codacy

lifflander requested review from PhilMiller, pnstickne, nmm0 and nlslatt June 4, 2020 23:41

lifflander self-assigned this Jun 5, 2020