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/18 00:13:05 UTC
[kudu-CR] rpc: micro-optimize delayed task handling
Hello Michael Ho, Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/9048
to review the following change.
Change subject: rpc: micro-optimize delayed task handling
......................................................................
rpc: micro-optimize delayed task handling
Previously we put all pending DelayedTasks in an STL set<>. However, we
don't really need a set -- using an intrusive doubly linked list is
sufficient and provides O(1) removal instead of O(lg n). This speeds up
the new unit test significantly.
I measured the new test using the following command line before and
after three times:
periodic-test --gtest_filter=\*Perf\* --gtest_repeat=10 2>&1 | grep User | tee -a /tmp/after
and then ran a t-test on the difference in CPU time:
data: d.before and d.after
t = 19.097, df = 48.327, p-value < 2.2e-16
alternative hypothesis: true difference in means is not equal to 0
95 percent confidence interval:
0.09643424 0.11912596
sample estimates:
mean of x mean of y
0.4324359 0.3246558
So this saves a noticeable amount of CPU when there are a lot of pending
tasks.
Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
---
M src/kudu/rpc/periodic-test.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
3 files changed, 46 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/9048/1
--
To view, visit http://gerrit.cloudera.org:8080/9048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
Gerrit-Change-Number: 9048
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
[kudu-CR] rpc: micro-optimize delayed task handling
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9048 )
Change subject: rpc: micro-optimize delayed task handling
......................................................................
rpc: micro-optimize delayed task handling
Previously we put all pending DelayedTasks in an STL set<>. However, we
don't really need a set -- using an intrusive doubly linked list is
sufficient and provides O(1) removal instead of O(lg n). This speeds up
the new unit test significantly.
I measured the new test using the following command line before and
after three times:
periodic-test --gtest_filter=\*Perf\* --gtest_repeat=10 2>&1 | grep User | tee -a /tmp/after
and then ran a t-test on the difference in CPU time:
data: d.before and d.after
t = 19.097, df = 48.327, p-value < 2.2e-16
alternative hypothesis: true difference in means is not equal to 0
95 percent confidence interval:
0.09643424 0.11912596
sample estimates:
mean of x mean of y
0.4324359 0.3246558
So this saves a noticeable amount of CPU when there are a lot of pending
tasks.
Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
Reviewed-on: http://gerrit.cloudera.org:8080/9048
Tested-by: Kudu Jenkins
Reviewed-by: Michael Ho <kw...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/rpc/periodic-test.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
3 files changed, 48 insertions(+), 10 deletions(-)
Approvals:
Kudu Jenkins: Verified
Michael Ho: Looks good to me, but someone else must approve
Todd Lipcon: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/9048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
Gerrit-Change-Number: 9048
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] rpc: micro-optimize delayed task handling
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9048 )
Change subject: rpc: micro-optimize delayed task handling
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/9048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
Gerrit-Change-Number: 9048
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 02 Feb 2018 20:30:12 +0000
Gerrit-HasComments: No
[kudu-CR] rpc: micro-optimize delayed task handling
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9048 )
Change subject: rpc: micro-optimize delayed task handling
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9048/3/src/kudu/rpc/periodic-test.cc
File src/kudu/rpc/periodic-test.cc:
http://gerrit.cloudera.org:8080/#/c/9048/3/src/kudu/rpc/periodic-test.cc@270
PS3, Line 270: SCOPED_CLEANUP({ messenger->Shutdown(); });
> Just for my own education: any reason why we use SCOPED_CLEANUP() here inst
We usually use this pattern for any place where we set up something that needs special cleanup. That way if someone added any early return down below (eg an ASSERT) we'd be guaranteed that the cleanup would run.
--
To view, visit http://gerrit.cloudera.org:8080/9048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
Gerrit-Change-Number: 9048
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 31 Jan 2018 01:47:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] rpc: micro-optimize delayed task handling
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9048
to look at the new patch set (#2).
Change subject: rpc: micro-optimize delayed task handling
......................................................................
rpc: micro-optimize delayed task handling
Previously we put all pending DelayedTasks in an STL set<>. However, we
don't really need a set -- using an intrusive doubly linked list is
sufficient and provides O(1) removal instead of O(lg n). This speeds up
the new unit test significantly.
I measured the new test using the following command line before and
after three times:
periodic-test --gtest_filter=\*Perf\* --gtest_repeat=10 2>&1 | grep User | tee -a /tmp/after
and then ran a t-test on the difference in CPU time:
data: d.before and d.after
t = 19.097, df = 48.327, p-value < 2.2e-16
alternative hypothesis: true difference in means is not equal to 0
95 percent confidence interval:
0.09643424 0.11912596
sample estimates:
mean of x mean of y
0.4324359 0.3246558
So this saves a noticeable amount of CPU when there are a lot of pending
tasks.
Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
---
M src/kudu/rpc/periodic-test.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
3 files changed, 44 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/9048/2
--
To view, visit http://gerrit.cloudera.org:8080/9048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
Gerrit-Change-Number: 9048
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] rpc: micro-optimize delayed task handling
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9048 )
Change subject: rpc: micro-optimize delayed task handling
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9048/1/src/kudu/rpc/periodic-test.cc
File src/kudu/rpc/periodic-test.cc:
http://gerrit.cloudera.org:8080/#/c/9048/1/src/kudu/rpc/periodic-test.cc@274
PS1, Line 274: [&] {
: // No-op.
: },
Nit: combine into one line:
[&] {}, // no-op
http://gerrit.cloudera.org:8080/#/c/9048/1/src/kudu/rpc/periodic-test.cc@278
PS1, Line 278: PeriodicTimer::Options()));
You can omit this.
--
To view, visit http://gerrit.cloudera.org:8080/9048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
Gerrit-Change-Number: 9048
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 18 Jan 2018 00:34:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] rpc: micro-optimize delayed task handling
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9048 )
Change subject: rpc: micro-optimize delayed task handling
......................................................................
Patch Set 3: Code-Review+2
Adar's out on PTO for several weeks and only had nits on rev 1, so I'm just going to make a judgment call that he'd be +2 on this
--
To view, visit http://gerrit.cloudera.org:8080/9048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
Gerrit-Change-Number: 9048
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 05 Feb 2018 04:21:32 +0000
Gerrit-HasComments: No
[kudu-CR] rpc: micro-optimize delayed task handling
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9048 )
Change subject: rpc: micro-optimize delayed task handling
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9048/3/src/kudu/rpc/periodic-test.cc
File src/kudu/rpc/periodic-test.cc:
http://gerrit.cloudera.org:8080/#/c/9048/3/src/kudu/rpc/periodic-test.cc@270
PS3, Line 270: SCOPED_CLEANUP({ messenger->Shutdown(); });
Just for my own education: any reason why we use SCOPED_CLEANUP() here instead of calling messenger->Shutdown() at the end of the function ?
I don't see any early return in this function.
--
To view, visit http://gerrit.cloudera.org:8080/9048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
Gerrit-Change-Number: 9048
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jan 2018 08:50:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] rpc: micro-optimize delayed task handling
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9048 )
Change subject: rpc: micro-optimize delayed task handling
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9048/1/src/kudu/rpc/periodic-test.cc
File src/kudu/rpc/periodic-test.cc:
http://gerrit.cloudera.org:8080/#/c/9048/1/src/kudu/rpc/periodic-test.cc@274
PS1, Line 274: [&] {
: // No-op.
: },
> Nit: combine into one line:
Done
http://gerrit.cloudera.org:8080/#/c/9048/1/src/kudu/rpc/periodic-test.cc@278
PS1, Line 278: PeriodicTimer::Options()));
> You can omit this.
Done
http://gerrit.cloudera.org:8080/#/c/9048/1/src/kudu/rpc/reactor.cc
File src/kudu/rpc/reactor.cc:
http://gerrit.cloudera.org:8080/#/c/9048/1/src/kudu/rpc/reactor.cc@695
PS1, Line 695: void DelayedTask::TimerHandler(ev::timer& watcher, int revents) {
> warning: parameter 'watcher' is unused [misc-unused-parameters]
Done
--
To view, visit http://gerrit.cloudera.org:8080/9048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
Gerrit-Change-Number: 9048
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jan 2018 02:29:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] rpc: micro-optimize delayed task handling
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9048
to look at the new patch set (#3).
Change subject: rpc: micro-optimize delayed task handling
......................................................................
rpc: micro-optimize delayed task handling
Previously we put all pending DelayedTasks in an STL set<>. However, we
don't really need a set -- using an intrusive doubly linked list is
sufficient and provides O(1) removal instead of O(lg n). This speeds up
the new unit test significantly.
I measured the new test using the following command line before and
after three times:
periodic-test --gtest_filter=\*Perf\* --gtest_repeat=10 2>&1 | grep User | tee -a /tmp/after
and then ran a t-test on the difference in CPU time:
data: d.before and d.after
t = 19.097, df = 48.327, p-value < 2.2e-16
alternative hypothesis: true difference in means is not equal to 0
95 percent confidence interval:
0.09643424 0.11912596
sample estimates:
mean of x mean of y
0.4324359 0.3246558
So this saves a noticeable amount of CPU when there are a lot of pending
tasks.
Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
---
M src/kudu/rpc/periodic-test.cc
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
3 files changed, 48 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/9048/3
--
To view, visit http://gerrit.cloudera.org:8080/9048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b6be5ef7e8f464f3bc4c62f904e2692b30ddc65
Gerrit-Change-Number: 9048
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>