[kudu-CR] rpc: micro-optimize delayed task handling
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 LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 05 Feb 2018 04:21:32 + Gerrit-HasComments: No
[kudu-CR] rpc: micro-optimize delayed task handling
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 HoReviewed-by: Todd Lipcon --- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] rpc: micro-optimize delayed task handling
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 LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 02 Feb 2018 20:30:12 + Gerrit-HasComments: No
[kudu-CR] rpc: micro-optimize delayed task handling
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 LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 31 Jan 2018 01:47:59 + Gerrit-HasComments: Yes
[kudu-CR] rpc: micro-optimize delayed task handling
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 LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 24 Jan 2018 08:50:51 + Gerrit-HasComments: Yes
[kudu-CR] rpc: micro-optimize delayed task handling
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 LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] rpc: micro-optimize delayed task handling
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 LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 24 Jan 2018 02:29:33 + Gerrit-HasComments: Yes
[kudu-CR] rpc: micro-optimize delayed task handling
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 LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot
[kudu-CR] rpc: micro-optimize delayed task handling
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 LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Thu, 18 Jan 2018 00:34:40 + Gerrit-HasComments: Yes
[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 LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Michael Ho