Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GlobPtr and global memory space concept (GlobHeapMem, GlobStaticMem, ...) #333

Merged
merged 21 commits into from
Mar 24, 2017

Conversation

fuchsto
Copy link
Member

@fuchsto fuchsto commented Mar 17, 2017

Related to #325 #246

I'm trying to ease my discontent with pointer semantics of GlobPtr. In brief, I want it to comply to shared-memory pointer semantics.

A pointer in NUMA address space behaves nicely when it is incremented past a NUMA domain address space. Also, the distance between two pointers in separate NUMA domains (numa_dom_a_ptr - numa_dom_b_ptr) is well-defined.

The corresponding DASH global pointers, on the other hand, are not that polite at all. When a GlobPtr is incremented past the unit's local address range, it refers to an undefined address in global memory. Also, GlobPtr provides distance operators, but these only give sane results if both operands refer to the same unit memory space.

Implemented in this PR:

In the proposed implementation in this PR, GlobPtr depends on a global memory space (usually an instance of GlobMem or GlobDynamicMem, the latter to be renamed to GlobHeap for reasons I want to discuss next week).
The type dependency of GlobPtr to a pattern type is removed.

In some situations, a GlobPtr is initialized but no reference to a global memory space is available, for example in GlobRef constructors. A global pointer without a coupled global memory space is valid if no pointer arithmetics are performed on it.
In the vanilla shared-memory world, this corresponds to a const pointer (T * const, not const T * which is pointer-to-const).
For this, I introduced the GlobConstPtr concept which provides all operations of GlobPtr apart from those that would not be well-defined without a reference global memory space.
A GlobConstPtr can be explicitly converted to a local pointer. Pointer arithmetics on the converted local pointer then again are allowed as the restriction to local address space is obvious:

// Initialized from dart_gptr_t without reference to a GlobMem:
GlobConstPtr<T> gcp(some_dart_gptr);

++gcp; // fails, pointer arithmetics on const pointer not accessible

// Conversion to local pointer requires explicit cast / conversion op:
T * lptr = static_cast<T *>(gcp); // or: gcp.local();
// Application developer performed explicit conversion and definitely
// knows that the pointer's address space is limited to local memory:
++lptr; // works

Global const pointers also allow locality tests:

bool glob_const_ptr_is_local = GlobConstPtr<T>(some_dart_gptr).is_local();

The GlobMem concept is renamed to GlobalMemorySpace and extended by the following methods:

local_index_type GlobalMemorySpace::at(index_type global_index)
// -> { team_unit_t unit, index_type index }

size_type GlobalMemorySpace::local_size(unit = myid())

Global memory space maintains the local sizes of the associated units' local memory space.
As a result, GlobPtr is agnostic of iteration space (different from GlobIter types), but could traverse across the units' local memory, comparable to a bucket iterator.

The virtual global address translation required for robust global pointer arithmetics is illustrated in this article in the DASH wiki:

http://doc.dash-project.org/internal/Publications/DASH-GDM

glob_ptr_address_translation

@fuchsto fuchsto changed the title GlobPtr on global memory space, GlobConstPtr GlobPtr and global memory space concept (GlobHeapMem, GlobStaticMem, ...) Mar 18, 2017
Copy link
Member

@fmoessbauer fmoessbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good to me. Now we should have a clean memory interface.

@@ -0,0 +1,470 @@
#ifndef DASH__GLOB_UNIT_MEM_H__INCLUDED
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately you copied the content of the file istead of the file. So we lose the history.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's what git does when you git mv. Use git log --follow to see the full history.

@fuchsto fuchsto merged commit 15c107e into development Mar 24, 2017
@fuchsto
Copy link
Member Author

fuchsto commented Mar 24, 2017

@devreal @rkowalewski
To document the conclusion of our discussions earlier this week:

  • dart_gptr_incaddr could/should be removed as it's just a simplicistic wrapper for dart_gptr.addr += x and does not support traversal across unit address ranges

Sequential coherence of global address space can easily exceed the complexity we want to delegate to DART, therefore:

  • global pointer arithmetic and traversal across unit address ranges is in responsibility of DASH, more specific, the dash::GlobPtr types
  • maintaining the sizes of the single units' address ranges is in responsibility of memory spaces, more specific, GlobMemorySpace types like dash::GlobHeapMem, dash::GlobStaticMem, dash::GlobUnitMem

Also see #325

@rkowalewski
Copy link

rkowalewski commented Mar 25, 2017

dart_gptr_incaddr could/should be removed as it's just a simplicistic wrapper for dart_gptr.addr += x and does not support traversal across unit address ranges

After reviewing the changes regarding pointer arithmetic I am not happy with this for two reasons.

  • We have to expose the internal structure of a dart_gptr_t to DASH and possibly other users of DART. This is anything but good design. What happens if we modify the internal structure of a dart_gptr_t?
  • It is kind of inconsistent that we used to introduce DART types and now do not exploit them for address arithmetic. What if (sizeof(DART_TYPE) != sizeof(element_type))?

There may possibly be more issues about that.

@fuchsto
Copy link
Member Author

fuchsto commented Mar 25, 2017

dart_gptr_t is a public type and its structure is documented. If it changes, we will adapt DASH to it.
The stride for pointers in memory is not related to the DART types.
Otherwise, local memory could not be iterated in DASH, like:

dash::Array<int> array(...);
array.local[4] += 4; // <- you see my point. DART size is irrelevant.

@fuchsto
Copy link
Member Author

fuchsto commented Mar 25, 2017

... so we rely on MPI/DART to detect type sizes correctly.
But you still are right in principle: If type sizes differ between DART and C++, we are in trouble.
Not sure if we validate this at the moment. We should.

@rkowalewski
Copy link

Pointer arithmetics on the local view. Yes you are right, maybe I was thinking too complicated. And in general we can still overload the corresponding operators in C++. But that will be a different issue.

Regarding the documentation of dart_gptr_t. OK, but anyway. It can be an error-prone effort to refactor a large code base if the internal structure has, let's say, more complicated changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants