Henry Robinson has posted comments on this change. Change subject: Improve AtomicInt abstraction and implementation ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/2573/1/be/src/common/atomic.h File be/src/common/atomic.h: Line 65: value_(initial) > It's not even guaranteed to be atomic, because no other thread should be re fair enough, I think it's fine to leave it. Line 101: > Yeah, I was thinking about introducing AtomicInt32, AtomicInt64 in another AtomicInt32 is better, as it adheres to our existing convention for integer sizes, plus the reasons you mentioned. Fine not to do it now. 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 > Add() in AtomicInt<> specifies it has (full) barrier semantics (which it do Nope, just mistakenly translated this as a straight store. That's maybe one point in favour of encoding the memory semantics into the method names (which presumably we'll need to do if we have different memory semantics for the same operation ever). But again, fine not to do it here. 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() > Hmm, thanks for pointing that out. I didn't notice it. I think I'll rewrite Sure thing. Line 152: } > I don't think so. we don't expect it to be prevented from making progress o fair enough. -- 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
