You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Dan Hecht (Code Review)" <ge...@cloudera.org> on 2018/06/25 18:09:54 UTC
[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 <dh...@cloudera.com>
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 25 Jun 2018 18:49:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 25 Jun 2018 18:30:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 25 Jun 2018 18:58:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 25 Jun 2018 22:25:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Comment-Date: Mon, 25 Jun 2018 18:13:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Comment-Date: Mon, 25 Jun 2018 18:13:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 25 Jun 2018 18:54:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Comment-Date: Mon, 25 Jun 2018 18:45:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 25 Jun 2018 18:52:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
[Impala-ASF-CR] IMPALA-7207: make Coordinator::exec state an atomic enum
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <dh...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 25 Jun 2018 18:56:06 +0000
Gerrit-HasComments: No