[kudu-CR] cfile: make encoder/decoder classes final

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: cfile: make encoder/decoder classes final
..


Patch Set 4: Verified+1

Test failure was Java test leaving tmp minikdc output, unrelated.

-- 
To view, visit http://gerrit.cloudera.org:8080/5202
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0b8337868330b43595a275e837a448e3ba0c066
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] cfile: make encoder/decoder classes final

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

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 ObjectSymbol
 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 ObjectSymbol
 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
Reviewed-on: http://gerrit.cloudera.org:8080/5202
Reviewed-by: Dan Burkert 
Tested-by: Todd Lipcon 
---
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(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Todd Lipcon: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5202
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia0b8337868330b43595a275e837a448e3ba0c066
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] cfile: make encoder/decoder classes final

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: cfile: make encoder/decoder classes final
..


Patch Set 4: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5202
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0b8337868330b43595a275e837a448e3ba0c066
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] cfile: make encoder/decoder classes final

[Todd Lipcon (Code Review)]

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 ObjectSymbol
 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 ObjectSymbol
 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon