You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2018/01/22 16:20:42 UTC
[kudu-CR] consensus: avoid extra thread wakeups for Peer::SignalRequest
Hello Mike Percy,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/9092
to review the following change.
Change subject: consensus: avoid extra thread wakeups for Peer::SignalRequest
......................................................................
consensus: avoid extra thread wakeups for Peer::SignalRequest
Looking at a profile on a YCSB workload I see a significant amount of
CPU in submitting tasks to the raft threadpool for requests. My guess is
that, in a workload like this, we often call SignalRequest while the
request was already pending.
This patch avoids submitting the threadpool task if there is already a
task pending.
Change-Id: I62ec4ea91e976590ce2de4fa34db61eaf9aab92a
---
M src/kudu/consensus/consensus_peers.cc
1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/9092/1
--
To view, visit http://gerrit.cloudera.org:8080/9092
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I62ec4ea91e976590ce2de4fa34db61eaf9aab92a
Gerrit-Change-Number: 9092
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] consensus: avoid extra thread wakeups for Peer::SignalRequest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9092 )
Change subject: consensus: avoid extra thread wakeups for Peer::SignalRequest
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9092/2/src/kudu/consensus/consensus_peers.cc
File src/kudu/consensus/consensus_peers.cc:
http://gerrit.cloudera.org:8080/#/c/9092/2/src/kudu/consensus/consensus_peers.cc@172
PS2, Line 172: request_pending_
> worth it to add an exception for "even_if_queue_empty"? thinking of TOCTOU,
It's possible that it changes, but since this isn't synchronized against the change as is, then it's always possible that this current code would submit the task, and the task would run before it changes.
In other words, in the current code, we already could run the task below immediately, in which case we would see the request_pending_==true and skip, right?
--
To view, visit http://gerrit.cloudera.org:8080/9092
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62ec4ea91e976590ce2de4fa34db61eaf9aab92a
Gerrit-Change-Number: 9092
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 23 Jan 2018 21:40:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus: avoid extra thread wakeups for Peer::SignalRequest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Mike Percy, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9092
to look at the new patch set (#2).
Change subject: consensus: avoid extra thread wakeups for Peer::SignalRequest
......................................................................
consensus: avoid extra thread wakeups for Peer::SignalRequest
Looking at a profile on a YCSB workload I see a significant amount of
CPU in submitting tasks to the raft threadpool for requests. My guess is
that, in a workload like this, we often call SignalRequest while the
request was already pending.
This patch avoids submitting the threadpool task if there is already a
task pending.
Change-Id: I62ec4ea91e976590ce2de4fa34db61eaf9aab92a
---
M src/kudu/consensus/consensus_peers.cc
1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/9092/2
--
To view, visit http://gerrit.cloudera.org:8080/9092
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I62ec4ea91e976590ce2de4fa34db61eaf9aab92a
Gerrit-Change-Number: 9092
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] consensus: avoid extra thread wakeups for Peer::SignalRequest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/9092 )
Change subject: consensus: avoid extra thread wakeups for Peer::SignalRequest
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9092/2/src/kudu/consensus/consensus_peers.cc
File src/kudu/consensus/consensus_peers.cc:
http://gerrit.cloudera.org:8080/#/c/9092/2/src/kudu/consensus/consensus_peers.cc@172
PS2, Line 172: request_pending_
> It's possible that it changes, but since this isn't synchronized against th
My concern was mostly whether there might be some subtle side-effect of calling sendnextrequest even with a a pending request, but I now see that's not the case.
--
To view, visit http://gerrit.cloudera.org:8080/9092
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62ec4ea91e976590ce2de4fa34db61eaf9aab92a
Gerrit-Change-Number: 9092
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jan 2018 19:04:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus: avoid extra thread wakeups for Peer::SignalRequest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9092 )
Change subject: consensus: avoid extra thread wakeups for Peer::SignalRequest
......................................................................
consensus: avoid extra thread wakeups for Peer::SignalRequest
Looking at a profile on a YCSB workload I see a significant amount of
CPU in submitting tasks to the raft threadpool for requests. My guess is
that, in a workload like this, we often call SignalRequest while the
request was already pending.
This patch avoids submitting the threadpool task if there is already a
task pending.
Change-Id: I62ec4ea91e976590ce2de4fa34db61eaf9aab92a
Reviewed-on: http://gerrit.cloudera.org:8080/9092
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves <da...@gmail.com>
---
M src/kudu/consensus/consensus_peers.cc
1 file changed, 6 insertions(+), 0 deletions(-)
Approvals:
Kudu Jenkins: Verified
David Ribeiro Alves: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/9092
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I62ec4ea91e976590ce2de4fa34db61eaf9aab92a
Gerrit-Change-Number: 9092
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: avoid extra thread wakeups for Peer::SignalRequest
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/9092 )
Change subject: consensus: avoid extra thread wakeups for Peer::SignalRequest
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9092/2/src/kudu/consensus/consensus_peers.cc
File src/kudu/consensus/consensus_peers.cc:
http://gerrit.cloudera.org:8080/#/c/9092/2/src/kudu/consensus/consensus_peers.cc@172
PS2, Line 172: request_pending_
worth it to add an exception for "even_if_queue_empty"? thinking of TOCTOU, when request_pending_ is true here but isn't true anymore when SendNextRequest is called.
--
To view, visit http://gerrit.cloudera.org:8080/9092
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62ec4ea91e976590ce2de4fa34db61eaf9aab92a
Gerrit-Change-Number: 9092
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 23 Jan 2018 21:35:32 +0000
Gerrit-HasComments: Yes