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/23 16:59:34 UTC
[kudu-CR] binary plain block: avoid a virtual call in hot path
Todd Lipcon has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5202
Change subject: binary_plain_block: avoid a virtual call in hot path
......................................................................
binary_plain_block: avoid a virtual call in hot path
This sped up writing 100M strings in cfile-test a few percent on my
machine.
Change-Id: Ia0b8337868330b43595a275e837a448e3ba0c066
---
M src/kudu/cfile/binary_plain_block.cc
1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/5202/1
--
To view, visit http://gerrit.cloudera.org:8080/5202
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia0b8337868330b43595a275e837a448e3ba0c066
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
[kudu-CR] binary plain block: avoid a virtual call in hot path
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Adar Dembo, 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 (#2).
Change subject: binary_plain_block: avoid a virtual call in hot path
......................................................................
binary_plain_block: avoid a virtual call in hot path
This sped up writing 100M strings in cfile-test a few percent on my
machine.
Change-Id: Ia0b8337868330b43595a275e837a448e3ba0c066
---
M src/kudu/cfile/binary_plain_block.cc
1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/5202/2
--
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: 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 <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] cfile: make encoder/decoder classes final
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <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>
Gerrit-HasComments: No
[kudu-CR] cfile: make encoder/decoder classes final
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 <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>
Gerrit-HasComments: No
[kudu-CR] binary plain block: avoid a virtual call in hot path
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: binary_plain_block: avoid a virtual call in hot path
......................................................................
Patch Set 3: 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: 3
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-HasComments: No
[kudu-CR] cfile: make encoder/decoder classes final
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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>
[kudu-CR] binary plain block: avoid a virtual call in hot path
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: binary_plain_block: avoid a virtual call in hot path
......................................................................
Patch Set 3: Code-Review-1
Dan and I actaully just chatted, and it sounds like a better solution with equivalent perf results may be to mark these encoder classes as 'final', so the compiler knows it can devirtualize such calls.
--
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: 3
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>
Gerrit-HasComments: No
[kudu-CR] cfile: make encoder/decoder classes final
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 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
Reviewed-on: http://gerrit.cloudera.org:8080/5202
Reviewed-by: Dan Burkert <da...@apache.org>
Tested-by: Todd Lipcon <to...@apache.org>
---
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 <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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>