Merge ported delegates #23

bholt · 2013-01-29T06:12:49Z

I'd like to just merge in the ported delegates to our porting branch so I can start from there to do the cache next.

I've put in a temporary alias (#define'd send_heap_message over send_message) to distinguish messages that shouldn't block (replies), until we are able to grep and replace them with the real call. Let me know if you'd like to change the name or method of aliasing or whatever.

With your approval I'll merge it in.

(note: the previous pull request was closed when I merged in common-namespacing hence the new pull request)

This is just to help us distinguish cases that shouldn't block until we have actual heap-allocated (or whatever) messages.

bholt · 2013-01-29T07:21:20Z

@nelsonje std::cout references are gone in the version at the head of this pull request.

In a pull request you can comment on the diff or individual commits. This does raise a question of whether we want to merge in the original commits as is or rebase/squash them into fewer saner commits before merging...

nelsonje · 2013-01-29T08:06:38Z

system/Delegate.hpp

+    bool write(GlobalAddress<T> target, U value) {
+      // TODO: don't return any val, requires changes to `delegate::call()`.
+      return call(target.node(), [target, value]() -> bool {
+        *target.pointer() = (T)value;


I'm not sure this is the best interface. Is there a reason the value argument is type U, not type T? The downside of this approach is that the C-style cast is done implicitly, which may cause data to be silently lost (if T is int8_t and U is int64_t, for example).

Yeah, the different U template type was to allow for implicit conversions. I didn't realize it wouldn't warn if it would truncate. I'll change it.

@nelsonje Do you think "fetch_and_add" should allow the increment value to be of a different type? This would allow one to overload the "+" operator to take some other type (if desired) and still use this delegate.

Hmm. That sounds reasonable, although a bit non-intuitive. The fact that it returns a copy of the T makes me wonder how useful it would be.

The best approach may be to leave the T/U interface everywhere and let an implicit cast happen at the assignment instead of the call if allowed. I think they're essentially equivalent.

A use case I can think of is incrementing a pointer or GlobalAddress.

What do you mean at the assignment? Do the coersion inside the delegate
still? I think the GlobalAddress type should tell you what space is
available to be read or written to.

On Tuesday, January 29, 2013, Jacob Nelson wrote:

In system/Delegate.hpp:

/// round-trip communication is complete.

/// @warning { Target object must lie on a single node (not span blocks in global address space). }

template< typename T >

T read(GlobalAddress target) {

return call(target.node(), [target]() -> T {

return *target.pointer();

});

}

/// Blocking remote write.

/// @warning { Target object must lie on a single node (not span blocks in global address space). }

template< typename T, typename U >

bool write(GlobalAddress target, U value) {

// TODO: don't return any val, requires changes to `delegate::call()`.

return call(target.node(), [target, value]() -> bool {

*target.pointer() = (T)value;

Hmm. That sounds reasonable, although a bit non-intuitive. The fact that
it returns a copy of the T makes me wonder how useful it would be.

The best approach may be to leave the T/U interface everywhere and let an
implicit cast happen at the assignment instead of the call if allowed. I
think they're essentially equivalent.

—
Reply to this email directly or view it on GitHubhttps://github.com/nelsonje/grappa/pull/23/files#r2806175.

(there are no pattern match problems with the T/U signature)

Okay I removed the C-style cast, thank you for the discussion on this.

Okay, great. Unfortunately -Wconversion is kind of loud, especially inside
gasnet. I don't know that we'll be able to enable it.

On Tue, Jan 29, 2013 at 11:37 AM, Brandon Holt notifications@github.comwrote:

In system/Delegate.hpp:

/// round-trip communication is complete.

/// @warning { Target object must lie on a single node (not span blocks in global address space). }

template< typename T >

T read(GlobalAddress target) {

return call(target.node(), [target]() -> T {

return *target.pointer();

});

}

/// Blocking remote write.

/// @warning { Target object must lie on a single node (not span blocks in global address space). }

template< typename T, typename U >

bool write(GlobalAddress target, U value) {

// TODO: don't return any val, requires changes to `delegate::call()`.

return call(target.node(), [target, value]() -> bool {

*target.pointer() = (T)value;

Okay I removed the C-style cast.

—
Reply to this email directly or view it on GitHubhttps://github.com/nelsonje/grappa/pull/23/files#r2814383.

We can tell GCC to treat the Gasnet headers as "system headers" and not issue warnings in them. Turns out we've accumulated a good deal of our own warnings, but we could squash those.

http://gcc.gnu.org/onlinedocs/cpp/System-Headers.html

Aha. That's an interesting idea. My only concern would be that we'd force
our users to deal with warnings they don't want to see as well.

On Tue, Jan 29, 2013 at 12:20 PM, Brandon Holt notifications@github.comwrote:

In system/Delegate.hpp:

/// round-trip communication is complete.

/// @warning { Target object must lie on a single node (not span blocks in global address space). }

template< typename T >

T read(GlobalAddress target) {

return call(target.node(), [target]() -> T {

return *target.pointer();

});

}

/// Blocking remote write.

/// @warning { Target object must lie on a single node (not span blocks in global address space). }

template< typename T, typename U >

bool write(GlobalAddress target, U value) {

// TODO: don't return any val, requires changes to `delegate::call()`.

return call(target.node(), [target, value]() -> bool {

*target.pointer() = (T)value;

We can tell GCC to treat the Gasnet headers as "system headers" and not
issue warnings in them. Turns out we've accumulated a good deal of our own
warnings, but we could squash those.

http://gcc.gnu.org/onlinedocs/cpp/System-Headers.html

—
Reply to this email directly or view it on GitHubhttps://github.com/nelsonje/grappa/pull/23/files#r2815168.

bmyerz · 2013-01-29T18:59:42Z

system/Delegate.hpp

+    /// function pointer, or functor object) is called from the `dest` core and the return
+    /// value is sent back to the calling task.
+    template <typename F>
+    inline auto call(Core dest, F func) -> decltype(func()) {


I expect we will want a non-blocking version. I'll have to think about whether it is possible to not have write two versions of read/write/cmp-swap/...

I'm still not sure what the difference is between non-blocking delegates and caches are, except the case where you don't care about a return value at all, but then you have to distinguish whether you want to block until a delegate completes or not (for consistency).

I think the difference is in how writes are handled (maybe reads too). With delegates, we expect both to generate a remote reference for every access.

Probably the right way to do this is to either pass in or return (with appropriate anti-move protection) the result FullEmpty<>; then the user can just block on that if necessary.

What if we started a new issue to discuss and incorporate these changes in the future?

It's not clear that we need all of this functionality to get things up and running (we don't have these abilities already) and without messages we don't have to block on, we can't implement something like feed-forward delegates anyway.

100% agree.

On Tue, Jan 29, 2013 at 12:32 PM, Brandon Holt notifications@github.comwrote:

In system/Delegate.hpp:

+#include "Grappa.hpp"
+#include "Message.hpp"
+#include "FullEmpty.hpp"
+#include "Message.hpp"
+#include "ConditionVariable.hpp"
+
+namespace Grappa {

namespace delegate {

/// @addtogroup Delegates

/// @{

/// Implements essentially a blocking remote procedure call. Callable object (lambda,

/// function pointer, or functor object) is called from the dest core and the return

/// value is sent back to the calling task.

template

inline auto call(Core dest, F func) -> decltype(func()) {

What if we started a new issue to discuss and incorporate these changes in
the future?

It's not clear that we need all of this functionality to get things up and
running (we don't have these abilities already) and without messages we
don't have to block on, we can't implement something like feed-forward
delegates anyway.

—
Reply to this email directly or view it on GitHubhttps://github.com/nelsonje/grappa/pull/23/files#r2815389.

…ersion?)

Merge ported delegates

bholt and others added 12 commits January 28, 2013 17:44

Add delegate tests.

38a7ac8

Trying to get get Delegates to work.

786f5a2

delegate_call to call a function remotely and return the value

a81d32a

Fixed segfault (was taking address of copy rather than original)

5103ab4

Also seems to work by passing result through by reference.

75f7da9

Working generic delegate, uses decltype for return.

c1c46c4

Implement delegate_read using delegate_call and test.

d57ea45

Move new delegates to Delegate.hpp, minimal testing.

4e581d5

Fix ConditionVariable GlobalAddress wait target node.

9fd23ef

Add documentation to new Delegates and make tests less embarrassing.

070566f

Use Grappa::mycore()

3bf126c

Temporary alias send_heap_message -> send_message

efdea49

This is just to help us distinguish cases that shouldn't block until we have actual heap-allocated (or whatever) messages.

ghost assigned bholt Jan 29, 2013

nelsonje reviewed Jan 29, 2013
View reviewed changes

Remove experimental design code from earlier.

b1a98b0

bmyerz reviewed Jan 29, 2013
View reviewed changes

Get rid of explicit C-style cast, count on implicit casts (and -Wconv…

1458629

…ersion?)

bholt mentioned this pull request Jan 29, 2013

Asynchronous delegates #24

Closed

bholt added 2 commits January 29, 2013 13:58

Put in calls to existing stats.

3610362

Add calls to latency stats.

52f9337

bholt added a commit that referenced this pull request Jan 29, 2013

Merge pull request #23 from nelsonje/bholt-delegates

d107af2

Merge ported delegates

bholt merged commit d107af2 into new-messages-old-aggregator Jan 29, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge ported delegates #23

Merge ported delegates #23

bholt commented Jan 29, 2013

bholt commented Jan 29, 2013

nelsonje Jan 29, 2013

bholt Jan 29, 2013

bholt Jan 29, 2013

nelsonje Jan 29, 2013

bholt Jan 29, 2013

nelsonje Jan 29, 2013

bholt Jan 29, 2013

nelsonje Jan 29, 2013

bholt Jan 29, 2013

nelsonje Jan 29, 2013

bmyerz Jan 29, 2013

bholt Jan 29, 2013

nelsonje Jan 29, 2013

bholt Jan 29, 2013

nelsonje Jan 29, 2013

Merge ported delegates #23

Merge ported delegates #23

Conversation

bholt commented Jan 29, 2013

bholt commented Jan 29, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment