Sailesh Mukil 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? 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 in this case is "strictly" better than using spin locks? Atomic variables cause cache invalidations across CPUs, and lock the corresponding cache line in the corresponding CPU. Load() and Store() operations on this atomic enum happen much more often than calls to ReturnedAllResults(), so I was wondering if we're actually improving anything by doing this or not. Not a major issue, I'm just thinking out loud. -- 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 <dhe...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Mon, 25 Jun 2018 18:30:36 +0000 Gerrit-HasComments: Yes
