-
Notifications
You must be signed in to change notification settings - Fork 48
Fix GlobAsyncRef #447
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
Fix GlobAsyncRef #447
Conversation
|
Update: I also removed the comparison operators to avoid hidden synchronous I noticed that |
fmoessbauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks really good. The interface changes are all reasonable and required. Thanks for putting time into that.
Just one note: The move construction of GlobAsyncRef is defaulted, but there is no move-assignment operator. IMO we can default this as well. However I have not thought about possible impact, especially if the target GlobAsyncRef should be flushed before the assignment.
Regarding your question in the comments: Yes, I vote for changing the behavior of GlobRef as well.
And last but not least, we should check that all dash-apps still compile.
| } | ||
| * but the value referenced by \c new_value can be re-used immediately. | ||
| */ | ||
| void set(const_value_type& new_value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this renaming break anything? Should we provide an alias put()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check. IMO, put() is closer to a C PGAS API.
| /** | ||
| * Complete all outstanding non-blocking operations executed by the | ||
| * local unit on the narray's underlying global memory. | ||
| * Complete all outstanding non-blocking operations to the specified unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool feature. This fixes / answers #416.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely, unfortunately. We still need a beefed up dash::Future for bulk wait to not leak memory through the handles. It's on my list :)
This PR is (yet another) attempt to get
GlobAsyncRefinto a stable state. As discussed at the F2F, value semantics are removed and no consistency guarantees are provided, i.e., a read does not necessarily yield a previously written value unless there has been a flush in between.Asynchronous writes use a temporary value and a handle, which is used to ensure local completion upon destruction of the
GlobAsyncRefobject. Reads are never asynchronous as that would require speculatively starting a transfer even if theGlobAsyncRefwould be used for writing only. With value semantics removed, this is now explicit by callingget().The following use-case is covered:
What does not work:
The PR also fixes the semantics of
dash::Matrix::flush*anddash::Array::flush*and removespushandfetch, which have not been used.Related: #357 #358 #409
Fixes #365
Closes #371 #390