Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10811 )
Change subject: IMPALA-7207: make Coordinator::exec_state_ an atomic enum ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/10811/1/be/src/common/atomic.h File be/src/common/atomic.h: http://gerrit.cloudera.org:8080/#/c/10811/1/be/src/common/atomic.h@156 PS1, Line 156: static_cast<int32_t> > Since they're enums, shouldn't implicit casts work? No, because it's an "enum class" and so no implicit conversions. http://gerrit.cloudera.org:8080/#/c/10811/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/10811/2/be/src/runtime/coordinator.cc@a511 PS2, Line 511: : : : > I've not done an analysis, but is it fair to say that using an atomic enum For x86, Load() and Store() only needs to ensure that the memory access emitted by the compiler occurs with a single instruction (and prevent compiler reordering). For x86, the CPU always ensures that (aligned) loads or stores are atomic. i.e. no lck prefix is needed. (See atomicops-internals-x86.h which is what it ultimately boils down to.) So, should be strictly better since there is no real runtime overhead on x86 to using atomic load/store. (That wouldn't necessarily be true if it was load-modify-write, i.e. cmpxchg). -- To view, visit http://gerrit.cloudera.org:8080/10811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac Gerrit-Change-Number: 10811 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Hecht <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Mon, 25 Jun 2018 18:45:18 +0000 Gerrit-HasComments: Yes
