impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) Improve AtomicInt abstraction and implementation
Date Thu, 17 Mar 2016 23:24:57 GMT
Dan Hecht has posted comments on this change.

Change subject: Improve AtomicInt abstraction and implementation

Patch Set 1:


Thanks for the review! I had a couple questions for you so we can converge faster before preparing
the next patch.
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.
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?
File be/src/scheduling/

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.
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: If2dc1b258ad7ef7ddc3db4e78de3065012cfa923
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Dan Hecht <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message