[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10811 ) Change subject: IMPALA-7207: make Coordinator::exec_state_ an atomic enum .. IMPALA-7207: make Coordinator::exec_state_ an atomic enum That allows us to avoid taking the lock in cases where only the exec_state_ field needs to be read (as opposed to needing to read both exec_state_ and exec_status_). In particular, it avoids the lock on the non-terminating paths, which is the common case. Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac Reviewed-on: http://gerrit.cloudera.org:8080/10811 Reviewed-by: Tim Armstrong Tested-by: Impala Public Jenkins --- M be/src/common/atomic.h M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/runtime/fragment-instance-state.h 4 files changed, 19 insertions(+), 20 deletions(-) Approvals: Tim Armstrong: Looks good to me, approved Impala Public Jenkins: Verified -- 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: merged Gerrit-Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac Gerrit-Change-Number: 10811 Gerrit-PatchSet: 4 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Impala Public Jenkins 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 3: Verified+1 -- 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: 3 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 25 Jun 2018 22:25:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Impala Public Jenkins 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 3: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2736/ DRY_RUN=false -- 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: 3 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 25 Jun 2018 18:58:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Tim Armstrong 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 3: Code-Review+2 -- 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: 3 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 25 Jun 2018 18:56:06 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
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 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/10811/2/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10811/2/be/src/runtime/coordinator.h@278 PS2, Line 278: exec_state_ > exec_state_ Done http://gerrit.cloudera.org:8080/#/c/10811/2/be/src/runtime/coordinator.h@279 PS2, Line 279: both : // fields > does this mean reading both fields atomically? Wording is a little unclear. let me know if that clarifies, or if not what might help. -- 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: 3 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 25 Jun 2018 18:54:36 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Hello Sailesh Mukil, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10811 to look at the new patch set (#3). Change subject: IMPALA-7207: make Coordinator::exec_state_ an atomic enum .. IMPALA-7207: make Coordinator::exec_state_ an atomic enum That allows us to avoid taking the lock in cases where only the exec_state_ field needs to be read (as opposed to needing to read both exec_state_ and exec_status_). In particular, it avoids the lock on the non-terminating paths, which is the common case. Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac --- M be/src/common/atomic.h M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/runtime/fragment-instance-state.h 4 files changed, 19 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/10811/3 -- 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: newpatchset Gerrit-Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac Gerrit-Change-Number: 10811 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Tim Armstrong 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/2/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10811/2/be/src/runtime/coordinator.h@278 PS2, Line 278: exec-state_ exec_state_ http://gerrit.cloudera.org:8080/#/c/10811/2/be/src/runtime/coordinator.h@279 PS2, Line 279: both : // fields. does this mean reading both fields atomically? Wording is a little unclear. -- 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 Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 25 Jun 2018 18:52:38 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
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: Code-Review+1 (1 comment) 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: : : : > For x86, Load() and Store() only needs to ensure that the memory access emi Thanks! I wasn't aware of that. -- 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 Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Mon, 25 Jun 2018 18:49:29 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
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 > 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 Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Mon, 25 Jun 2018 18:45:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
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 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 Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Mon, 25 Jun 2018 18:30:36 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
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: (1 comment) http://gerrit.cloudera.org:8080/#/c/10811/1/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10811/1/be/src/runtime/coordinator.h@278 PS1, Line 278: exec_status_. exec_state_ can be read i > update that. Done -- 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 Gerrit-Reviewer: Dan Hecht Gerrit-Comment-Date: Mon, 25 Jun 2018 18:13:37 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Dan Hecht has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/10811 ) Change subject: IMPALA-7207: make Coordinator::exec_state_ an atomic enum .. IMPALA-7207: make Coordinator::exec_state_ an atomic enum That allows us to avoid taking the lock in cases where only the exec_state_ field needs to be read (as opposed to needing to read both exec_state_ and exec_status_). In particular, it avoids the lock on the non-terminating paths, which is the common case. Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac --- M be/src/common/atomic.h M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/runtime/fragment-instance-state.h 4 files changed, 19 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/10811/2 -- 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: newpatchset Gerrit-Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac Gerrit-Change-Number: 10811 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
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 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10811/1/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10811/1/be/src/runtime/coordinator.h@278 PS1, Line 278: // protects exec-state_ and exec_status_ update that. -- 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: 1 Gerrit-Owner: Dan Hecht Gerrit-Reviewer: Dan Hecht Gerrit-Comment-Date: Mon, 25 Jun 2018 18:13:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Dan Hecht has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10811 Change subject: IMPALA-7207: make Coordinator::exec_state_ an atomic enum .. IMPALA-7207: make Coordinator::exec_state_ an atomic enum That allows us to avoid taking the lock in cases where only the exec_state_ field needs to be read (as opposed to needing to read both exec_state_ and exec_status_). In particular, it avoids the lock on the non-terminating paths, which is the common case. Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac --- M be/src/common/atomic.h M be/src/runtime/coordinator.cc M be/src/runtime/coordinator.h M be/src/runtime/fragment-instance-state.h 4 files changed, 15 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/10811/1 -- 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: newchange Gerrit-Change-Id: Ie6c5d5c6ccfdfd533cd0607aab6f554e664b90ac Gerrit-Change-Number: 10811 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Hecht