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

Copy / Assignment of dash::Atomic #289

Open
fuchsto opened this issue Feb 17, 2017 · 7 comments
Open

Copy / Assignment of dash::Atomic #289

fuchsto opened this issue Feb 17, 2017 · 7 comments

Comments

@fuchsto
Copy link
Member

fuchsto commented Feb 17, 2017

Question on dash::Atomic:

Why is the copy constructor deleted, but assignment is not?

  • operator=(T) is deleted
  • operator=(const self_t &) is implicitly defaulted

That can't be right
When I explicitly delete assignment, tests don't compile

Also, the constructor Atomic(T) is not marked explicit, so it's an implicit conversion constructor
What's the intended semantics here?

The current implementation contradicts the documentation in dash/include/dash/Atomic.h

@fuchsto
Copy link
Member Author

fuchsto commented Feb 17, 2017

For now, I added

self_t & operator=(const self_t & other) = default;

But assignment is still explicitly deleted, so this is not well-defined.

@fmoessbauer
Copy link
Member

Yes, that should be fixed. From the type-safety side it would be best to disable all forms of assignment, because the member is always set by DART using memory copy. But if all assignments are disabled, the dash algorithms like dash::fill will not work anymore, as they use local assignment internally.

AFAIK we have the following options:

  • make assignment private and use friend
  • allow assignment and live with *(array.lbegin()) = 5 being possible

@fuchsto
Copy link
Member Author

fuchsto commented Feb 19, 2017

It's actually to be expected that dash::fill would not work for dash::Atomic<T> containers value types.

Similarily, you can;t use std::fill for an STL std::vector<std::atomic<T>>.
So a container cannot be "filled" with atomics, they need to exist already. You would have to use emplace_back (not implying you should, though).

In general terms: semantics have priority over algorithm convenience. Especially for non-trivial types like atomics. Assignment and conversion (oh my!) should not be allowed.

We might be able to make dash::for_each work to initialize a dash::Array<dash::Atomic<T>> with a function that uses elem.store instead of assignment.

@fmoessbauer
Copy link
Member

With some knowledge gained the last couple of month, I want to re-think the topic:

It's actually to be expected that dash::fill would not work for dash::Atomic<T> containers value types.

No, not really as the assignment operator of std::Atomic (T operator=( T desired )) is defined. This atomically replaces the value. Of course, that is not possible in DASH, as we have to use DART for atomically replacing the value at an DART address. However our implementation should provide a similar interface if possible.

Similarily, you can;t use std::fill for an STL std::vector<std::atomic<T>>.
So a container cannot be "filled" with atomics, they need to exist already.

That is only partially correct: std::fill works for atomic types, as it assigns the values. The only thing necessary to use std::vector<std::atomic<T>> is to construct the atomics in place, as they cannot be copied.

We might be able to make dash::for_each work to initialize a dash::Array<dash::Atomic<T>> with a function that uses elem.store instead of assignment.

Yes, and the way I intend to do this is by using iterator traits. With the stuff in PR #310 and #300 , it will be easy to switch the algorithm to use an non-localized version (working on the local part, but using DART) .

After merging these PRs, I will take care of fixing the implementation.

@fuchsto
Copy link
Member Author

fuchsto commented May 11, 2017

That is only partially correct: std::fill works for atomic types, as it assigns the values. The only thing
necessary to use std::vector<std::atomic> is to construct the atomics in place, as they cannot be
copied.

Yes, and as fill does not provide emplace the elements have to exist, that's what I wrote.
For example, std::filling an std::array<std::atomic<T>> would work as the elements are pre-initialized.
Where did I go wrong in my statement?

On using iterator traits to dictate the behavior of dash::for_each:
Obviously, iterator traits specify ... well, traits (properties) of an iterator type.
As iterators are exclusively about iteration, these traits can only possibly affect iteration semantics.

So if I got your point, you want to specialize dash::for_each for a new iterator trait, say "emplace_iterator_tag".
That already sounds really fishy, you must admit.
Iterators aren't even aware of const'ness of their referenced values, but now they even specify their assignment operations? I find it hard to rationalize.

First, what's the category of this trait? We would have to invent one. That is, there will be a new category of iterator properties which is orthogonal to all traits that actually specify iterator properties.
Perhaps my cold sweat is just the autism, but ...

Then: iterators, hence the name, are a concept for iteration. That's what the iterator tags in the STL all are about: to specify / restrict positioning in the iteration space.
Also, assuming we implement it like this, we would hide value semantics from the user. Values are assigned with different operation semantics and there is no way of knowing, apart from enable_ifing every interface.
Lastly, it would not work with STL algorithms as these don't respect non-STL traits.

But perhaps I got your point wrong in the first place.

PS: If the STL consistently does not allow a specific usage (like removing and returning an element in a single operation or turning an address into an atomic reference in mid-air), this is a strong indication that there are reasons not to do it.
It might be irrelevant reasons, but I think we should analyze this some more until we are confidently aware of them.

@fmoessbauer
Copy link
Member

Let me give a short explanation, as multiple people asked me about this issue. I'll try to keep it as brief as possible:

Basics

  • Elements in (all) DASH containers are accessed using a GlobRef<T>.
  • dash::Atomic<T> is a phantom type, just to specialize the GlobRef for using different put / get implementations. This is also the key problem, because all accesses to an atomic value have to be
    performed through a GlobRef.

No local accesses to a dash::Atomic

The atomicity of operations on an dash::Atomic element are guaranteed by using atomic DART functions (e.g. fetch_and_op). These work on a DART_GPTR, which is provided by the GlobRef, but not of the actual member dash::Atomic. The member is only a wrapped T and has no knowledge on the global address space (and it's address in it). Hence, the following will never work:

dash::Array<dash::Atomic> arr(10);
*(arr.lbegin()) = 1; // must not compile

To prohibit this bypassing of the atomic semantics, the corresponding operators are disabled for the atomic type.

In this context a differentiation between physical and logical local ranges is important, as pointed out in PR #371. While *(arr.lbegin()) : T offers physical access, *(arr.local.begin()) : GlobRef<T> offers logical access to the local range. For our Atomics, we have to prohibit physical access, because the atomic semantics are only ensured if the elements are accessed via GlobRef.

Performance tuning in DASH algorithms

For gaining performance, the DASH algorithms (e.g. dash::fill) use a short path by working on the physical local part, instead of the logical one. This has lead to various issues in the past, but is essential for achieving acceptable performance. However this is not possible for types (currently only dash::Atomic) which have to be accessed through a global reference.

How iterator traits can help

A common thing with all STL / DASH algorithms is that they take iterators as input. The properties of the iterators can then be accessed using traits, e.g. std::iterator_traits. The algorithm implementations can then be dispatch on these information. The STL traits provide the following information:

difference_type     Iterator::difference_type
value_type          Iterator::value_type
pointer	            Iterator::pointer
reference	    Iterator::reference
iterator_category   Iterator::iterator_category

This is the point where my idea kicks in: By using the value_type. If this type matches dash::Atomic<T>, the algorithm operates on the logical view. Otherwise, the physical range is processed. This also works with STL algorithms, as dash::GlobIter matches the STL iterator concept.

This is perfectly well-defined, as both algorithm variants generate the same output (for a given input).

Similar implementations in the STL/Boost

For example, std::fill behaves differently when operating on contiguous ranges of trivial elements (memset) or non-trivial elements (operator=). So this should be totally valid.

@fuchsto see this example for what I meant with std::fill on elements in a vector.

@fuchsto
Copy link
Member Author

fuchsto commented May 13, 2017

We discussed this live already, just for documentation: Yes! Looks correctly right to me, will be robust for STL algos.

Columbo turning around right before the door

Sir ... just one more question.

We didn't actually discuss alternatives to the GlobRef<Atomic>-phantom type trick.
It still is very ... aaahm ... "clever", in every interpretation of the word.
We still don't have value semantics well-defined, or has that been resolved in the meantime?

And now we put enable_if-duck tape on algorithms ... because of a GlobRef specialization ...

So, assuming there is no better alternative, we would have to add a enable_if-Guard to every interface of any function where GlobPtr / GlobIter are dereferenced to native pointers.

Really try to imagine the moment you explain this to an application developer with a straight face, in full color. And then really envision how you add "... and that's cool, because there is no way STL algorithms could break it!"

You know, I know, but I'm not sure the world is ready for this.
Let's really try to find a better way.

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

No branches or pull requests

4 participants