Dan Hecht has posted comments on this change. Change subject: Improve AtomicInt abstraction and implementation ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/2573/2/be/src/common/atomic-test.cc File be/src/common/atomic-test.cc: Line 57: int > nit: be explicit here - int32_t Done Line 183: reodrer > reorder Done Line 189: aquire > acquire Done Line 204: p = *payload; > how about setting *payload to 'it' in AcquireReleasethreadA()? Right now i Yeah, that's true that it is permissible for the compiler to optimize L184/190. Added a compiler barrier to guard against that loop optimization and also a small delay loop to encourage thread A to be ready for Phase 1 before thread B to give the most coverage. And used it/-it as the payload to verify they work in lockstep. http://gerrit.cloudera.org:8080/#/c/2573/2/be/src/common/atomic.h File be/src/common/atomic.h: Line 95: oldVal > old_val, new_val Done http://gerrit.cloudera.org:8080/#/c/2573/2/be/src/util/progress-updater.cc File be/src/util/progress-updater.cc: Line 59: if (new_percentage - old_percentage > update_period_) > this seems like a bug (not one you have to fix in this patch) - if there ar Yeah, given the above comment about out-of-order, I assumed we didn't really care about this case, and adding it to the if-stmt could make us "miss" some VLOGs (if multiple update_period_ worth of progress is racing). I'll think about doing a follow on change to tighten this up. http://gerrit.cloudera.org:8080/#/c/2573/2/be/src/util/progress-updater.h File be/src/util/progress-updater.h: Line 74: output > 'outputs'? 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: 2 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
