Dan Hecht has posted comments on this change. Change subject: Improve AtomicInt abstraction and implementation ......................................................................
Patch Set 1: (6 comments) Thanks for the review! I had a couple questions for you so we can converge faster before preparing the next patch. http://gerrit.cloudera.org:8080/#/c/2573/1/be/src/common/atomic.h File be/src/common/atomic.h: Line 65: value_(initial) > maybe comment that the constructor has NoBarrier semantics. It's not even guaranteed to be atomic, because no other thread should be referencing until after the construction is complete. but i can explicitly say that. Line 101: > How about Yeah, I was thinking about introducing AtomicInt32, AtomicInt64 in another commit and converting our current declarations, but iddn't want to do the conversion right now. Do you prefer Integer/Long over 32/64? Personally, I like the 32/64 since atomic operations are low level so exact sizing makes sense. But I could be convinced otherwise. BTW, also thinking about AtomicPtr<T> for that one case in runtime filters, but in a future change. http://gerrit.cloudera.org:8080/#/c/2573/1/be/src/runtime/disk-io-mgr-internal.h File be/src/runtime/disk-io-mgr-internal.h: Line 375: barrier > per the comment in atomic-util.h, this doesn't have barrier semantics but j Add() in AtomicInt<> specifies it has (full) barrier semantics (which it does). Or am I misunderstanding your comment? http://gerrit.cloudera.org:8080/#/c/2573/1/be/src/scheduling/query-resource-mgr.cc File be/src/scheduling/query-resource-mgr.cc: Line 265: if (thread_in_expand_->Add(0L) == 0L) { > NVM, presumably the full barrier is required to ensure that early_exit is p Yup, that's why I didn't want to waste time thinking about whether acquire was sufficient. So, I'd prefer to leave it alone too. http://gerrit.cloudera.org:8080/#/c/2573/1/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: Line 118: virtual double double_value() > comment that this doesn't have barrier semantics? Hmm, thanks for pointing that out. I didn't notice it. I think I'll rewrite it so it does use Load(): int64_t v = value_.Load(); return *reinterpret_cast<const double*>(&v); sound good? Line 152: } > should this loop pause the CPU? I don't think so. we don't expect it to be prevented from making progress on the next attempt -- it's not like the other thread leaves it in a state where the next attempt is likely to fail (unlike a spinlock acquire). -- To view, visit http://gerrit.cloudera.org:8080/2573 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If2dc1b258ad7ef7ddc3db4e78de3065012cfa923 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Dan Hecht <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
