fix: add missing barrier to fix reads in the coordinator fiber #1009
Conversation
Fixes #997. Signed-off-by: Roman Gershman <[email protected]>
|
My regression tests on ARM64 pass. |
|
|
||
| cb_ = std::move(cb); | ||
| cb_ptr_ = &cb; |
There was a problem hiding this comment.
Unless I'm missing something here, this is unsafe. You're taking a ref to a local var.
Perhaps you intended to copy it to a local member of a non-pointer type?
I realize you'd like to know if it's initialized, so you can have the member be of type std::optional
BTW note that this is somewhat risky, as now callers to this API must make sure that their callable outlive the inner object, but I don't know this API so it might be OK.
There was a problem hiding this comment.
The lifetime of cb_/cb_ptr_ is only for during of ScheduleSingleHop/Execute.
There was a problem hiding this comment.
It's true that we could use std::optional here but at the end I use FunctionRef to accept lambdas which by itself is valid only during the call of these functions.
There was a problem hiding this comment.
Sure, if the lifetime of cb_ptr_ is only for the duration of these methods then there's no issue. I didn't realize that, but reading the code now - it makes sense. Sorry about the noise :)
BTW, in the previous version (with std::function) the lambdas were moved into the Transaction object, so they could have theoretically had a longer lifetime than the function call. I'm not sure if that's what you implied in "... lambdas which by itself is valid only during the call of these functions", but anyway, that's obviously irrelevant.
There was a problem hiding this comment.
yes, they could - and we paid for it with unnecessary allocation, which I discovered when fixing the bug.
These allocations by themselves are not awful, but their pattern is - to always malloc in one thread and free in another. This puts an unnecessary load on malloc library to move free lists from thread to thread.
those cb_ = nullptr are the assignments that reset the function object.
| @@ -471,7 +478,7 @@ class Transaction { | |||
| // Reverse argument mapping for ReverseArgIndex to convert from shard index to original index. | |||
| std::vector<uint32_t> reverse_index_; | |||
|
|
|||
| RunnableType cb_; // Run on shard threads | |||
| RunnableType* cb_ptr_ = nullptr; // Run on shard threads | |||
There was a problem hiding this comment.
absl::FunctionRef already acts as a lightweight reference, so there is no need for the pointer in the general context.
However its always used as a pointer to the argument, so it makes sense like this
There was a problem hiding this comment.
It's not pissible to "reset" it or to clear it. I could use optional but decided to use a pointer instead.
run_count_so that the coordinator thread won't read stale data.FunctionRefthat does not allocate likestd::function.Fixes #997.