Henry Robinson has posted comments on this change. Change subject: Improve AtomicInt abstraction and implementation ......................................................................
Patch Set 1: (18 comments) http://gerrit.cloudera.org:8080/#/c/2573/1/be/src/common/atomic.h File be/src/common/atomic.h: Line 48: double space Line 48: nit: double space Line 49: /// is performed atomically and has a specified memory-ordering semantic: semantics (or change the comments on Load() and Store() to say semantic singular). Line 61: /// Suggest you explicitly tie these to the C++11 memory orders (http://en.cppreference.com/w/cpp/atomic/memory_order). Line 65: value_(initial) maybe comment that the constructor has NoBarrier semantics. Line 65: AtomicInt(T initial = 0) : value_(initial) {} Consider DCHECK(sizeof(T) == 4 || sizeof(T) == 8) (or static assert, but I suspect this file gets cross compiled). Line 101: How about typedef AtomicInt<int64_t> AtomicLong; typedef AtomicInt<int32_t> AtomicInteger; to encourage users not to specialise the class themselves? 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: int num_enabled = num_scanners_codegen_enabled_.Load(); int total = num_enabled + num_scanners_... AddRuntimeExecOption(Substitute("Codegen enabled: $0 out of $1", num_enabled, total)); 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 prefix, this works. Can you add a comment explaining why this is safe? 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 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 just release semantics (in C++11 the total barriers have acq_rel semantics, elsewhere I've seen them called "full" barriers). 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) { This should probably be Load() 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: DCHECK_GE(...) << "Init() should have been called already" 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 output? Line 43: spewed while you're here... s/spewed/printed? Line 71: double space 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? Line 152: } should this loop pause the CPU? -- 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
