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>