Rework Container Type Traits and GlobRef Conversion Rules

[rkowalewski]

Unfortunately, we cannot implement the following example:

struct Parent {
  int x;
};
struct Child : public Parent {
  int y;
};
static_assert(dash::is_container_compatible_v<Child>:, "FAILS");
//The reason is...
static_assert(std::is_standard_layout_v<Child>, "FAILS");
//... see https://en.cppreference.com/w/cpp/named_req/StandardLayoutType for more details, 
// especially the example with class inheritance.

// At the same time,
static_assert(std::is_trivially_copyable_v<Child>, "PASSES");

AFAIK we chose these constraints because we want to be able to do a simple memcpy in DART by using Shared Memory Windows. But std::memcpy() does not require a StandardLayoutType. A TrviallyCopyable type suffices as documented. So we should weaken our container requirements to allow a dash::Array<Child>.

We reworked GlobRef some time ago and the relatively strict requirements lead to one consequence which we have to deal with. We should permit the following conversion:

  dash::Array<Child> array{100};

  //...Initialize with whatever

  // This is a child and we read 8 bytes...
  auto asChild = array[10].get();
  /*
   * Here we explicitly cast it as Parent. In consequence, we read only 4
   * bytes (i.e., sizeof Parent).
   */
  auto asParent = static_cast<dash::GlobRef<Parent>>(array[10]).get();

EDIT: My suggestion is to replace is_standard_layout with is_trivially_default_constructible. Then, a Child is container compatible and this should suffice for our requirements. Thoughts on this?

[devreal]

AFAIK we chose these constraints because we want to be able to do a simple memcpy in DART by using Shared Memory Windows. But std::memcpy() does not require a StandardLayoutType.

There are two different things: we need is_trivially_copyable for any copying in and out of the DASH containers (has nothing to do with shared memory, we just cannot run an assignment operator when copying from unit A to unit B). The restriction on is_standard_layout did not come from me (in fact, I don't remember having seen it before :D) but there seems to be a good reason:

From https://en.cppreference.com/w/cpp/named_req/StandardLayoutType:

Has no virtual functions or virtual base classes

This restriction is important as we cannot copy vtables across process boundaries (unless we force things like ALSR to be disabled). Afaics, it doesn't include is_trivially_default_constructible but that is important since we don't run c'tors on the elements in the container (maybe I'm missing that in the definition of the standard layout type?)

So, to amend your proposal: !is_polymorphic<T> && is_trivially_default_constructible<T> && is_trivially_copyable<T> ?

[rkowalewski]

There are two different things: we need is_trivially_copyable for any copying in and out of the DASH containers (has nothing to do with shared memory, we just cannot run an assignment operator when copying from unit A to unit B).

That is what I meant, actually. In other words, we want to keep compatibility to C, MPI, etc.

Has no virtual functions or virtual base classes.

Correct. vtables shouldn't be allowed. But a standard layout gives a much stronger guarantee. We can include ! is_polymorphic<T>, however, we do not really need it. A polymorphic class is never trivially copyable.

[devreal]

You're right, is_trivially_copyable includes !is_polymorphic. Please ignore !is_polymorphic then :)

[rkowalewski]

Minor change: We cannot rely on is_trivially_default_constructible but only on is_trivially_copyable. As an example, is_trivially_default_constructible does not permit default initialization. This also excludes dash::Pair which has been valid until now.

And after thinking again we do not really need trivial construction. All we need is default construction which gives us still the possiblity to construct elements while constructing the container, although we do not really consider this currently.

So in summary:

template <class T>
struct is_container_compatible :
  public std::integral_constant<bool,
              std::is_default_constructible<T>::value
              && std::is_trivially_copyable<T>::value
         >
{ };

[codecov]

Codecov Report

Merging #630 into development will increase coverage by 0.01%.
The diff coverage is 96.29%.

@@               Coverage Diff               @@
##           development     #630      +/-   ##
===============================================
+ Coverage        84.83%   84.85%   +0.01%     
===============================================
  Files              335      336       +1     
  Lines            24798    24856      +58     
  Branches         11249    11282      +33     
===============================================
+ Hits             21038    21091      +53     
  Misses            3729     3729              
- Partials            31       36       +5

Impacted Files	Coverage Δ
dash/test/iterator/GlobAsyncRefTest.cc	95.78% <ø> (-0.48%)	⬇️
dash/include/dash/Coarray.h	100% <ø> (ø)	⬆️
dash/include/dash/io/hdf5/StorageDriver.h	100% <ø> (ø)	⬆️
dash/include/dash/Types.h	100% <ø> (ø)	⬆️
dash/include/dash/algorithm/Fill.h	100% <ø> (ø)	⬆️
dash/include/dash/memory/GlobStaticMem.h	90.78% <ø> (-0.12%)	⬇️
dart-impl/mpi/src/dart_communication.c	69.91% <ø> (ø)	⬆️
dash/include/dash/algorithm/SUMMA.h	96.55% <ø> (-0.05%)	⬇️
dash/include/dash/iterator/GlobIter.h	96.03% <100%> (ø)	⬆️
dash/include/dash/Shared.h	94.66% <100%> (ø)	⬆️
... and 15 more

[devreal]

For the record: default construction of elements in containers has been brought up in #479.

[rkowalewski]

For the record: default construction of elements in containers has been brought up in #479.

We discussed it already in a F2F and agreed on not doing that right? It is currently done only for dash::Shared. For the others we have to keep in mind NUMA #615.

[rkowalewski]

Like always, such PR get larger than expected. I am documenting everything now with sweet static_asserts in our test suite. These rules show if we can convert GlobRef<T> to GlobRef<U> by considering all const modifiers.

While this approach demonstrates it for GlobRef we can adapt it for GlobIter, GlobPtr and other friends.

[devreal]

I guess I'm missing something here, but why do we disallow explicit const->non-const conversion?

[devreal]

Sorry, this PR completely slipped through. Just a minor question, seems fine to me otherwise