Dan Hecht has posted comments on this change. Change subject: Improve AtomicInt abstraction and implementation ......................................................................
Patch Set 1: (16 comments) http://gerrit.cloudera.org:8080/#/c/2573/1/be/src/common/atomic.h File be/src/common/atomic.h: Line 48: > nit: double space Done Line 48: > double space old habit. fixed. Line 49: /// is performed atomically and has a specified memory-ordering semantic: > semantics (or change the comments on Load() and Store() to say semantic sin Made the others singular. Line 61: /// > Suggest you explicitly tie these to the C++11 memory orders (http://en.cppr Okay. "Barrier" in gutil/atomicops doesn't imply total ordering, though the implementation on x86 does. So for now, I claim "Barrier" is only acq_rel, but if/when we switch to c++11 as the underlying implementation we can consider upgrading to seq_cst (since it comes for free in read-modify-write operations on x86). (Though we don't currently have any uses that require seq_cst). Line 65: AtomicInt(T initial = 0) : value_(initial) {} > Consider DCHECK(sizeof(T) == 4 || sizeof(T) == 8) (or static assert, but I Done Line 101: > AtomicInt32 is better, as it adheres to our existing convention for integer Will do this in a follow on change. http://gerrit.cloudera.org:8080/#/c/2573/1/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: Line 1310: ss << "Codegen enabled: " << num_scanners_codegen_enabled_.Load() << " out of " : << (num_scanners_codegen_enabled_.Load() + num_scanners_codegen_disabled_.Load()); > could use a cleanup: Done http://gerrit.cloudera.org:8080/#/c/2573/1/be/src/gutil/atomicops-internals-x86.h File be/src/gutil/atomicops-internals-x86.h: Line 484: No > This is a bit subtle: just because NoBarrier_CAS is implemented with a LOCK Done. Added the comment a few lines above since the other routines were already making this assumption. http://gerrit.cloudera.org:8080/#/c/2573/1/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: Line 459: // Once remote fragments are started, they can start making ReportExecStatus RPCs, > blank line before this Personally, I prefer not because the function is already tough to fit on one screen, but okay. 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 327: __sync_synchronize > Will change this to MemoryBarrier() Done http://gerrit.cloudera.org:8080/#/c/2573/1/be/src/util/progress-updater.cc File be/src/util/progress-updater.cc: Line 40: DCHECK_GE(total_, 0); // Init() should have been called already. > instead of making the explanation a comment, pipe it to DCHECK_GE: Done (here and above). http://gerrit.cloudera.org:8080/#/c/2573/1/be/src/util/progress-updater.h File be/src/util/progress-updater.h: Line 41: label > can you 'quote' the parameter names so that they show up in the doxygen out Done Line 43: spewed > while you're here... s/spewed/printed? Done Line 71: > double space Done 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() > Sure thing. Done Line 164: Set > Will comment it's atomic Done -- 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
