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 2019/02/27 00:15:47 UTC
[kudu-CR] KUDU-2711 (part 2): use a RWMutex for TSDescriptor
Hello Will Berkeley,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/12614
to review the following change.
Change subject: KUDU-2711 (part 2): use a RWMutex for TSDescriptor
......................................................................
KUDU-2711 (part 2): use a RWMutex for TSDescriptor
One reason that GetTableLocations can end up slow is contention on the
TSDescriptor object.
This patch changes the spinlock to a RWMutex.
The benchmark improves throughput by about 33%:
table_locations-itest --gtest_filter=\*Bench\* \
--benchmark_num_tablets 300 --benchmark_num_threads 30 \
--benchmark_runtime_secs=10
before fix:
Count: 16622
Mean: 5365.24
Percentiles:
0% (min) = 42
25% = 4928
50% (med) = 5280
75% = 5760
95% = 6368
99% = 6720
99.9% = 7904
99.99% = 8704
100% (max) = 9870
with rwlock:
Count: 22179
Mean: 3901.19
Percentiles:
0% (min) = 52
25% = 3600
50% (med) = 3904
75% = 4192
95% = 4608
99% = 4896
99.9% = 5760
99.99% = 10048
100% (max) = 10661
Change-Id: Id48635e49f11d20fa1802b17a3ff5771632dfd01
---
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
2 files changed, 21 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/12614/1
--
To view, visit http://gerrit.cloudera.org:8080/12614
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id48635e49f11d20fa1802b17a3ff5771632dfd01
Gerrit-Change-Number: 12614
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2711 (part 2): use a RWMutex for TSDescriptor
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12614 )
Change subject: KUDU-2711 (part 2): use a RWMutex for TSDescriptor
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12614/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12614/1//COMMIT_MSG@12
PS1, Line 12: This patch changes the spinlock to an rw_spinlock.
> I agree with that. I tested with both an RWMutex and an rw_spinlock on ve05
The numbers in the commit message paint a rosier picture. Comparing the 'count' from RWMutex to rw_spinlock shows a ~15% improvement. Curious where you got the 5% from?
--
To view, visit http://gerrit.cloudera.org:8080/12614
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id48635e49f11d20fa1802b17a3ff5771632dfd01
Gerrit-Change-Number: 12614
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 08 Mar 2019 00:12:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2711 (part 2): use a RWMutex for TSDescriptor
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12614 )
Change subject: KUDU-2711 (part 2): use a RWMutex for TSDescriptor
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12614/2/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/12614/2/src/kudu/master/ts_descriptor.cc@310
PS2, Line 310: ts_admin_proxy_.reset(new tserver::TabletServerAdminServiceProxy(
: messenger, addr, std::move(host)));
nit: maybe,allocate new TabletServerAdminServiceProxy() outside of the lock and to ts_admin_proxy_ under the lock? It might help to save a few cycles of contention. Also, if that's a first call to GetLoggedInUser(), the benefits are much greater.
http://gerrit.cloudera.org:8080/#/c/12614/2/src/kudu/master/ts_descriptor.cc@333
PS2, Line 333: new consensus::ConsensusServiceProxy(
: messenger, addr, std::move(host))
nit: maybe, allocate new ConsensusServiceProxy() outside of the locked scope and put the consensus_proxy_.reset() under the lock only? That's not much, but it could save a few hundred (?) cycles.
--
To view, visit http://gerrit.cloudera.org:8080/12614
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id48635e49f11d20fa1802b17a3ff5771632dfd01
Gerrit-Change-Number: 12614
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 08 Mar 2019 00:36:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2711 (part 2): use a RWMutex for TSDescriptor
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12614 )
Change subject: KUDU-2711 (part 2): use a RWMutex for TSDescriptor
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12614/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12614/1//COMMIT_MSG@12
PS1, Line 12: This patch changes the spinlock to a RWMutex.
> Curious why you chose RWMutex and not rw_spinlock. There's no I/O here, so
I agree with that. I tested with both an RWMutex and an rw_spinlock on ve0518 and the rw_spinlock was about 5% better.
http://gerrit.cloudera.org:8080/#/c/12614/1//COMMIT_MSG@45
PS1, Line 45: 99.99% = 10048
> Interesting that the latency at the end of the tail increased slightly. Is
Not clear. In general, the variance of some "extreme" sort of sample variable can be (a lot) higher than the variance of a measure of central tendency like the median or mean.
See e.g. https://stats.stackexchange.com/questions/15970/what-is-the-variance-of-the-maximum-of-a-sample, which doesn't cover extreme percentiles but it does talk about the maximum and the order statistics.
--
To view, visit http://gerrit.cloudera.org:8080/12614
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id48635e49f11d20fa1802b17a3ff5771632dfd01
Gerrit-Change-Number: 12614
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 22:44:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2711 (part 2): use a RWMutex for TSDescriptor
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12614 )
Change subject: KUDU-2711 (part 2): use a RWMutex for TSDescriptor
......................................................................
KUDU-2711 (part 2): use a RWMutex for TSDescriptor
One reason that GetTableLocations can end up slow is contention on the
TSDescriptor object.
This patch changes the spinlock to an rw_spinlock.
The benchmark improves throughput by about 77%:
table_locations-itest --gtest_filter=\*Bench\* \
--benchmark_num_tablets 300 --benchmark_num_threads 30 \
--benchmark_runtime_secs=10
before fix:
Count: 10555
Mean: 8567.29
Percentiles:
0% (min) = 68
25% = 7712
50% (med) = 8320
75% = 9152
95% = 10880
99% = 12224
99.9% = 17024
99.99% = 27776
100% (max) = 30384
with rwlock:
Count: 18758
Mean: 4024.09
Percentiles:
0% (min) = 136
25% = 3472
50% (med) = 4032
75% = 4512
95% = 5248
99% = 6048
99.9% = 7712
99.99% = 13440
100% (max) = 13976
Change-Id: Id48635e49f11d20fa1802b17a3ff5771632dfd01
Reviewed-on: http://gerrit.cloudera.org:8080/12614
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
2 files changed, 20 insertions(+), 19 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, approved
Todd Lipcon: Looks good to me, approved
Alexey Serbin: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/12614
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id48635e49f11d20fa1802b17a3ff5771632dfd01
Gerrit-Change-Number: 12614
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2711 (part 2): use a RWMutex for TSDescriptor
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12614 )
Change subject: KUDU-2711 (part 2): use a RWMutex for TSDescriptor
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12614/2/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/12614/2/src/kudu/master/ts_descriptor.cc@333
PS2, Line 333: new consensus::ConsensusServiceProxy(
: messenger, addr, std::move(host))
> then you'd have to do another round of double-checked locking or something.
indeed :)
--
To view, visit http://gerrit.cloudera.org:8080/12614
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id48635e49f11d20fa1802b17a3ff5771632dfd01
Gerrit-Change-Number: 12614
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 08 Mar 2019 17:03:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2711 (part 2): use a RWMutex for TSDescriptor
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has uploaded a new patch set (#2) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/12614 )
Change subject: KUDU-2711 (part 2): use a RWMutex for TSDescriptor
......................................................................
KUDU-2711 (part 2): use a RWMutex for TSDescriptor
One reason that GetTableLocations can end up slow is contention on the
TSDescriptor object.
This patch changes the spinlock to an rw_spinlock.
The benchmark improves throughput by about 77%:
table_locations-itest --gtest_filter=\*Bench\* \
--benchmark_num_tablets 300 --benchmark_num_threads 30 \
--benchmark_runtime_secs=10
before fix:
Count: 10555
Mean: 8567.29
Percentiles:
0% (min) = 68
25% = 7712
50% (med) = 8320
75% = 9152
95% = 10880
99% = 12224
99.9% = 17024
99.99% = 27776
100% (max) = 30384
with rwlock:
Count: 18758
Mean: 4024.09
Percentiles:
0% (min) = 136
25% = 3472
50% (med) = 4032
75% = 4512
95% = 5248
99% = 6048
99.9% = 7712
99.99% = 13440
100% (max) = 13976
Change-Id: Id48635e49f11d20fa1802b17a3ff5771632dfd01
---
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
2 files changed, 20 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/12614/2
--
To view, visit http://gerrit.cloudera.org:8080/12614
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id48635e49f11d20fa1802b17a3ff5771632dfd01
Gerrit-Change-Number: 12614
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2711 (part 2): use a RWMutex for TSDescriptor
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12614 )
Change subject: KUDU-2711 (part 2): use a RWMutex for TSDescriptor
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12614/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12614/1//COMMIT_MSG@12
PS1, Line 12: This patch changes the spinlock to a RWMutex.
Curious why you chose RWMutex and not rw_spinlock. There's no I/O here, so I'd think a spinning lock would be optimal.
http://gerrit.cloudera.org:8080/#/c/12614/1//COMMIT_MSG@45
PS1, Line 45: 99.99% = 10048
Interesting that the latency at the end of the tail increased slightly. Is it just a small sample?
--
To view, visit http://gerrit.cloudera.org:8080/12614
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id48635e49f11d20fa1802b17a3ff5771632dfd01
Gerrit-Change-Number: 12614
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 27 Feb 2019 00:37:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2711 (part 2): use a RWMutex for TSDescriptor
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12614 )
Change subject: KUDU-2711 (part 2): use a RWMutex for TSDescriptor
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12614
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id48635e49f11d20fa1802b17a3ff5771632dfd01
Gerrit-Change-Number: 12614
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 08 Mar 2019 05:50:05 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2711 (part 2): use a RWMutex for TSDescriptor
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12614 )
Change subject: KUDU-2711 (part 2): use a RWMutex for TSDescriptor
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12614/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12614/1//COMMIT_MSG@12
PS1, Line 12: This patch changes the spinlock to an rw_spinlock.
> The numbers in the commit message paint a rosier picture. Comparing the 'co
Seems hard to compare my numbers vs Will's numbers. Not sure we ran the benchmarks on the same host
http://gerrit.cloudera.org:8080/#/c/12614/1//COMMIT_MSG@45
PS1, Line 45: 99.99% = 13440
> Not clear. In general, the variance of some "extreme" sort of sample variab
Agreed, the 99.99th percentile of 22000 samples is determined by just the two worst samples
http://gerrit.cloudera.org:8080/#/c/12614/2/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/12614/2/src/kudu/master/ts_descriptor.cc@333
PS2, Line 333: new consensus::ConsensusServiceProxy(
: messenger, addr, std::move(host))
> nit: maybe, allocate new ConsensusServiceProxy() outside of the locked scop
then you'd have to do another round of double-checked locking or something. I think this code path is cold enough it's not worth it
--
To view, visit http://gerrit.cloudera.org:8080/12614
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id48635e49f11d20fa1802b17a3ff5771632dfd01
Gerrit-Change-Number: 12614
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 08 Mar 2019 05:49:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2711 (part 2): use a RWMutex for TSDescriptor
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12614 )
Change subject: KUDU-2711 (part 2): use a RWMutex for TSDescriptor
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12614/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12614/1//COMMIT_MSG@12
PS1, Line 12: This patch changes the spinlock to a RWMutex.
> Seems hard to compare my numbers vs Will's numbers. Not sure we ran the ben
Yeah, we can't compare my numbers and Todd's. Also, the 5% comes from a third benchmark I ran whose numbers I didn't include here.
--
To view, visit http://gerrit.cloudera.org:8080/12614
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id48635e49f11d20fa1802b17a3ff5771632dfd01
Gerrit-Change-Number: 12614
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 11 Mar 2019 18:16:29 +0000
Gerrit-HasComments: Yes