Henry Robinson has posted comments on this change. Change subject: Improve AtomicInt abstraction and implementation ......................................................................
Patch Set 2: Code-Review+1 (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 Line 183: reodrer reorder Line 189: aquire acquire Line 204: p = *payload; how about setting *payload to 'it' in AcquireReleasethreadA()? Right now if you get lucky on the first loop, the store on line 184 could get omitted or reordered and we wouldn't notice. I'm not sure if the compiler's legitimately able to remove line 190 as a dead store, but if it was I think it might be able to hoist the store in line 184 out of the loop altogether. 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 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 are two concurrent calls to Update(1) (and update_period_ is large enough, so that new_percentage is the same in both cases), we'll get two lines printing X% complete for the same X. Not exactly a big deal, but can it be easily fixed by checking if we 'win' the CAS on line 61 after moving it to this line? 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'? -- 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
