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 2017/05/05 00:48:16 UTC
[kudu-CR] WIP: cfile set: avoid Status allocation for row not found
Hello David Ribeiro Alves,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/6804
to review the following change.
Change subject: WIP: cfile_set: avoid Status allocation for row not found
......................................................................
WIP: cfile_set: avoid Status allocation for row not found
We expect Bloom Filter lookups to typically return "row not found". The
code currently indicates this using a Status::NotFound(). However,
constructing such a status involves an alloc/free pair, which is
relatively expensive to do millions of times per second for this common
case.
In a profile of tcph_real_world, the tserver spends a good chunk of its
CPU in these code paths, especially now that our alloc/free contain
heavier instrumentation for memory accounting.
This changes to using a boost::optional<rowid_t> out-parameter for the
common case instead.
WIP: need to add benchmark results
Change-Id: I3056aaaf0eec6b7deebcd1f206cb551b75996927
---
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/diskrowset.cc
3 files changed, 30 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/6804/1
--
To view, visit http://gerrit.cloudera.org:8080/6804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3056aaaf0eec6b7deebcd1f206cb551b75996927
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
[kudu-CR] cfile set: avoid Status allocation for row not found
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6804
to look at the new patch set (#2).
Change subject: cfile_set: avoid Status allocation for row not found
......................................................................
cfile_set: avoid Status allocation for row not found
We expect Bloom Filter lookups to typically return "row not found". The
code currently indicates this using a Status::NotFound(). However,
constructing such a status involves an alloc/free pair, which is
relatively expensive to do millions of times per second for this common
case.
In a profile of tcph_real_world, the tserver spends a good chunk of its
CPU in these code paths, especially now that our alloc/free contain
heavier instrumentation for memory accounting.
This changes to using a boost::optional<rowid_t> out-parameter for the
common case instead.
Benchmarked with tpch_real_world SF300 as in change ID
I8b7e703c82ac14fbce3a699bdf6a2f0fb4ed93a1:
Before After
Mem rejections 62,790 75,318 (1.20x more)
Wall time 2490s 2185s (1.14x faster)
Server CPU 72,873s 61,879s (1.18x faster)
The increased mem rejections makes sense, since we're now able to insert
a bit faster while maintenance operations stayed the same speed. The CPU
improvements are due to this optimization, and the wall improvement is a
result of the CPU improvement.
Change-Id: I3056aaaf0eec6b7deebcd1f206cb551b75996927
---
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/diskrowset.cc
3 files changed, 33 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/6804/2
--
To view, visit http://gerrit.cloudera.org:8080/6804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3056aaaf0eec6b7deebcd1f206cb551b75996927
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] cfile set: avoid Status allocation for row not found
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: cfile_set: avoid Status allocation for row not found
......................................................................
cfile_set: avoid Status allocation for row not found
We expect Bloom Filter lookups to typically return "row not found". The
code currently indicates this using a Status::NotFound(). However,
constructing such a status involves an alloc/free pair, which is
relatively expensive to do millions of times per second for this common
case.
In a profile of tcph_real_world, the tserver spends a good chunk of its
CPU in these code paths, especially now that our alloc/free contain
heavier instrumentation for memory accounting.
This changes to using a boost::optional<rowid_t> out-parameter for the
common case instead.
Benchmarked with tpch_real_world SF300 as in change ID
I8b7e703c82ac14fbce3a699bdf6a2f0fb4ed93a1:
Before After
Mem rejections 62,790 75,318 (1.20x more)
Wall time 2490s 2185s (1.14x faster)
Server CPU 72,873s 61,879s (1.18x faster)
The increased mem rejections makes sense, since we're now able to insert
a bit faster while maintenance operations stayed the same speed. The CPU
improvements are due to this optimization, and the wall improvement is a
result of the CPU improvement.
Change-Id: I3056aaaf0eec6b7deebcd1f206cb551b75996927
Reviewed-on: http://gerrit.cloudera.org:8080/6804
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/diskrowset.cc
3 files changed, 33 insertions(+), 24 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I3056aaaf0eec6b7deebcd1f206cb551b75996927
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] cfile set: avoid Status allocation for row not found
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: cfile_set: avoid Status allocation for row not found
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6804/2/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:
PS2, Line 238: s.IsNotFound()
> If this is also a common case, it should be converted in the same way.
not that common, since the bloom filter above already cuts out 99.99% of cases or somesuch
http://gerrit.cloudera.org:8080/#/c/6804/2/src/kudu/tablet/diskrowset.cc
File src/kudu/tablet/diskrowset.cc:
Line 615: Status DiskRowSet::MutateRow(Timestamp timestamp,
> Likewise with the various NotFound() exits from here.
yea, that's a little trickier though, since we don't have a pre-existing out-param handy.
--
To view, visit http://gerrit.cloudera.org:8080/6804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3056aaaf0eec6b7deebcd1f206cb551b75996927
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] cfile set: avoid Status allocation for row not found
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: cfile_set: avoid Status allocation for row not found
......................................................................
Patch Set 2: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6804/2/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:
PS2, Line 238: s.IsNotFound()
If this is also a common case, it should be converted in the same way.
http://gerrit.cloudera.org:8080/#/c/6804/2/src/kudu/tablet/diskrowset.cc
File src/kudu/tablet/diskrowset.cc:
Line 615: Status DiskRowSet::MutateRow(Timestamp timestamp,
Likewise with the various NotFound() exits from here.
--
To view, visit http://gerrit.cloudera.org:8080/6804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3056aaaf0eec6b7deebcd1f206cb551b75996927
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] WIP: cfile set: avoid Status allocation for row not found
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: WIP: cfile_set: avoid Status allocation for row not found
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6804/1/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:
Line 221: } else if (!s.ok()) {
> warning: don't use else after return [readability-else-after-return]
Done
http://gerrit.cloudera.org:8080/#/c/6804/1/src/kudu/tablet/cfile_set.h
File src/kudu/tablet/cfile_set.h:
Line 111: Status NewKeyIterator(CFileIterator **iter) const;
> warning: function 'kudu::tablet::CFileSet::NewKeyIterator' has a definition
Done
--
To view, visit http://gerrit.cloudera.org:8080/6804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3056aaaf0eec6b7deebcd1f206cb551b75996927
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes