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 2016/11/30 01:47:33 UTC
[kudu-CR] cfile: make encoder/decoder classes final
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5202
to look at the new patch set (#4).
Change subject: cfile: make encoder/decoder classes final
......................................................................
cfile: make encoder/decoder classes final
Marking the classes final means that the compiler can be sure that a
virtual method call from within the class will always be satisfied by
the implementation in that same class (since no subclass could exist).
This increases inlining opportunities and other downstream
optimizations, and should improve performance a few percent in various
spots.
I verified the effect by running:
perf record ./build/latest/bin/cfile-test --gtest_filter=\*100MFileStringsPlain\*0
perf report -n | grep BinaryPlainBlockBuilder | c++filt
Before:
Overhead Samples Command Shared Object Symbol
6.22% 1943 cfile-test cfile-test [.] kudu::cfile::BinaryPlainBlockBuilder::Add(unsigned char const*, unsigned long)
1.23% 386 cfile-test cfile-test [.] kudu::cfile::BinaryPlainBlockBuilder::IsBlockFull() const
0.23% 73 cfile-test cfile-test [.] kudu::cfile::BinaryPlainBlockBuilder::Finish(unsigned int)
0.00% 1 cfile-test cfile-test [.] kudu::cfile::BinaryPlainBlockBuilder::Reset()
After:
Overhead Samples Command Shared Object Symbol
6.21% 1868 cfile-test cfile-test [.] kudu::cfile::BinaryPlainBlockBuilder::Add(unsigned char const*, unsigned long)
0.23% 69 cfile-test cfile-test [.] kudu::cfile::BinaryPlainBlockBuilder::Finish(unsigned int)
You can see that several functions were now inlined where they previously could
not be, and the sample count for 'Add' was also reduced.
Change-Id: Ia0b8337868330b43595a275e837a448e3ba0c066
---
M src/kudu/cfile/binary_dict_block.h
M src/kudu/cfile/binary_plain_block.h
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/cfile/binary_prefix_block.h
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/gvint_block.h
M src/kudu/cfile/plain_bitmap_block.h
M src/kudu/cfile/plain_block.h
M src/kudu/cfile/rle_block.h
9 files changed, 19 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/5202/4
--
To view, visit http://gerrit.cloudera.org:8080/5202
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia0b8337868330b43595a275e837a448e3ba0c066
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>