impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
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:

(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 <dhecht@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message