Unconstrained promise values #16
Conversation
| int kind; | ||
| volatile void *datum; | ||
| promise_kind_t kind; | ||
| void *volatile datum; |
There was a problem hiding this comment.
Does it make sense for the pointer itself to be volatile? It seems like its value will only ever be set once and never changed again, what's the case where having the pointer storage itself be volatile is necessary?
There was a problem hiding this comment.
I'm pretty sure the original author thought that volatile void* was the equivalent of a volatile reference in Java—but it's not. (The HC runtime was designed based on the HJ runtime, so it wouldn't surprise me if someone was just translating a Java class into a C struct and did this.)
Since these fields in the promise struct are what get modified concurrently (not the values they point to), I think the intent was to mark the pointers themselves volatile.
However, as I pointed out in #18, I think we should be able to rid our codebase of volatile altogether. Using __sync/__atomic builtins should already be providing the memory barriers that we need to avoid stale values, so I don't think volatile is doing anything useful...
I left these volatiles for now because I'm only pretty sure that we don't need them—but I'm 100% sure that they don't make any sense on the other side of the *.
There was an off-by-one error in some of the loops, causing gets on futures associated with promises that never had a put. There were a few other bad get operations as well, which I think may have been artifacts from before renaming hclib_get_future_for_promise.
The *pointers* should be volatile, not what they're pointing at. We should probably revisit all uses of "volatile" in the code at some later point, but this fixes some immediate type issues for accessing these fields in the promise struct.
This simplifies logic in several places where normal tasks are cast to dependent (future) tasks, and the allocation logic as well. We might want to add dependent tasks back later to save space, but this is working well for now, and the overhead is not significant. This is still a step in the right direction because we got rid of the triggered task struct (and the associated bookkeeping). Also removed unqualified [gs]et_current_finish functions, replacing with direct field accesses instead. These functions weren't static, but also weren't prefixed with hclib_, so it seemed easier to just drop them and do the field accesses directly instead.
We were using a sentinel value for the promise datum pointer, which meant you couldn't store arbitrary integers directly in the promise.
| if (future->owner->datum == UNINITIALIZED_PROMISE_DATA_PTR) { | ||
| return NULL; | ||
| } | ||
| return (void *)future->owner->datum; |
There was a problem hiding this comment.
This is related to the volatile void *datum change. This cast (and the similar casts in hclib_future_wait) are necessary because datum is a pointer to volatile data. After correcting the declaration, these casts are no longer necessary. The fact that we always discarded the volatile qualifier before dereferencing the datum pointer was a strong indicator that the declaration was incorrect.
You should be able to store whatever you want in a promise. The restriction on the promise payload value was carried over from the old HC runtime (although I'm not sure how necessary it was there either). We just check the
wait_list_headto see if a promise is satisfied. This might add a bit of overhead compared to directly checking the value ofdatum, but I wouldn't expect the difference to be significant (the two fields are probably on the same cache line).Managing all of the casts back and forth between the "dependent task" type and the normal task type was giving me a headache, so I collapsed both types back into the normal task type. We might want to split them up again later, but having just one task type while doing this refactoring meant one less thing that I had to worry about. Since the overhead of combining the two is just one pointer field, I don't think it will cause too much concern for now.
Both of these changes lay groundwork for changes in the C++ API in later commits, such as template specializations of the promise type, and code consolidation for task management.
There are also some fixes for bugs in the test suite that were uncovered while refactoring.
Note: This pull request builds on #15. You can compare the branches corresponding to the pull requests to see the diff between just #15 and this PR: cxx-cleanup-3...cxx-cleanup-4