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 <[email protected]>
Tested-by: Todd Lipcon <[email protected]>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/eb4b46b8
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/eb4b46b8
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/eb4b46b8
Branch: refs/heads/master
Commit: eb4b46b800a92d4964e5d0eaec4a4fd011463901
Parents: 6f42330
Author: Todd Lipcon <[email protected]>
Authored: Tue Nov 29 17:34:30 2016 -0800
Committer: Todd Lipcon <[email protected]>
Committed: Wed Nov 30 17:06:35 2016 +0000
----------------------------------------------------------------------
src/kudu/cfile/binary_dict_block.h | 4 ++--
src/kudu/cfile/binary_plain_block.h | 4 ++--
src/kudu/cfile/binary_prefix_block.cc | 5 +----
src/kudu/cfile/binary_prefix_block.h | 4 ++--
src/kudu/cfile/bshuf_block.h | 4 ++--
src/kudu/cfile/gvint_block.h | 4 ++--
src/kudu/cfile/plain_bitmap_block.h | 4 ++--
src/kudu/cfile/plain_block.h | 4 ++--
src/kudu/cfile/rle_block.h | 8 ++++----
9 files changed, 19 insertions(+), 22 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/eb4b46b8/src/kudu/cfile/binary_dict_block.h
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/binary_dict_block.h
b/src/kudu/cfile/binary_dict_block.h
index 95ff64c..c06a506 100644
--- a/src/kudu/cfile/binary_dict_block.h
+++ b/src/kudu/cfile/binary_dict_block.h
@@ -64,7 +64,7 @@ enum DictEncodingMode {
DictEncodingMode_max = 2
};
-class BinaryDictBlockBuilder : public BlockBuilder {
+class BinaryDictBlockBuilder final : public BlockBuilder {
public:
explicit BinaryDictBlockBuilder(const WriterOptions* options);
@@ -119,7 +119,7 @@ class BinaryDictBlockBuilder : public BlockBuilder {
class CFileIterator;
-class BinaryDictBlockDecoder : public BlockDecoder {
+class BinaryDictBlockDecoder final : public BlockDecoder {
public:
explicit BinaryDictBlockDecoder(Slice slice, CFileIterator* iter);
http://git-wip-us.apache.org/repos/asf/kudu/blob/eb4b46b8/src/kudu/cfile/binary_plain_block.h
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/binary_plain_block.h
b/src/kudu/cfile/binary_plain_block.h
index 1004596..8c9cd49 100644
--- a/src/kudu/cfile/binary_plain_block.h
+++ b/src/kudu/cfile/binary_plain_block.h
@@ -39,7 +39,7 @@ namespace cfile {
struct WriterOptions;
-class BinaryPlainBlockBuilder : public BlockBuilder {
+class BinaryPlainBlockBuilder final : public BlockBuilder {
public:
explicit BinaryPlainBlockBuilder(const WriterOptions *options);
@@ -88,7 +88,7 @@ class BinaryPlainBlockBuilder : public BlockBuilder {
};
-class BinaryPlainBlockDecoder : public BlockDecoder {
+class BinaryPlainBlockDecoder final : public BlockDecoder {
public:
explicit BinaryPlainBlockDecoder(Slice slice);
http://git-wip-us.apache.org/repos/asf/kudu/blob/eb4b46b8/src/kudu/cfile/binary_prefix_block.cc
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/binary_prefix_block.cc
b/src/kudu/cfile/binary_prefix_block.cc
index 0c299b0..faff8d9 100644
--- a/src/kudu/cfile/binary_prefix_block.cc
+++ b/src/kudu/cfile/binary_prefix_block.cc
@@ -130,10 +130,7 @@ int BinaryPrefixBlockBuilder::Add(const uint8_t *vals,
size_t count) {
int added = 0;
const Slice* slices = reinterpret_cast<const Slice*>(vals);
Slice prev_val(last_val_);
- // We generate a static call to IsBlockFull() to avoid the vtable lookup
- // in this hot path.
- while (!BinaryPrefixBlockBuilder::IsBlockFull() &&
- added < count) {
+ while (!IsBlockFull() && added < count) {
const Slice val = slices[added];
int old_size = buffer_.size();
http://git-wip-us.apache.org/repos/asf/kudu/blob/eb4b46b8/src/kudu/cfile/binary_prefix_block.h
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/binary_prefix_block.h
b/src/kudu/cfile/binary_prefix_block.h
index 3094b07..a330a09 100644
--- a/src/kudu/cfile/binary_prefix_block.h
+++ b/src/kudu/cfile/binary_prefix_block.h
@@ -33,7 +33,7 @@ struct WriterOptions;
// Encoding for data blocks of binary data that have common prefixes.
// This encodes in a manner similar to LevelDB (prefix coding)
-class BinaryPrefixBlockBuilder : public BlockBuilder {
+class BinaryPrefixBlockBuilder final : public BlockBuilder {
public:
explicit BinaryPrefixBlockBuilder(const WriterOptions *options);
@@ -82,7 +82,7 @@ class BinaryPrefixBlockBuilder : public BlockBuilder {
};
// Decoder for BINARY type, PREFIX encoding
-class BinaryPrefixBlockDecoder : public BlockDecoder {
+class BinaryPrefixBlockDecoder final : public BlockDecoder {
public:
explicit BinaryPrefixBlockDecoder(Slice slice);
http://git-wip-us.apache.org/repos/asf/kudu/blob/eb4b46b8/src/kudu/cfile/bshuf_block.h
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/bshuf_block.h b/src/kudu/cfile/bshuf_block.h
index 794da45..7ec0bae 100644
--- a/src/kudu/cfile/bshuf_block.h
+++ b/src/kudu/cfile/bshuf_block.h
@@ -82,7 +82,7 @@ void AbortWithBitShuffleError(int64_t val) ATTRIBUTE_NORETURN;
// The header is followed by the bitshuffle-compressed element data.
//
template<DataType Type>
-class BShufBlockBuilder : public BlockBuilder {
+class BShufBlockBuilder final : public BlockBuilder {
public:
explicit BShufBlockBuilder(const WriterOptions* options)
: count_(0),
@@ -215,7 +215,7 @@ template<>
Slice BShufBlockBuilder<UINT32>::Finish(rowid_t ordinal_pos);
template<DataType Type>
-class BShufBlockDecoder : public BlockDecoder {
+class BShufBlockDecoder final : public BlockDecoder {
public:
explicit BShufBlockDecoder(Slice slice)
: data_(std::move(slice)),
http://git-wip-us.apache.org/repos/asf/kudu/blob/eb4b46b8/src/kudu/cfile/gvint_block.h
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/gvint_block.h b/src/kudu/cfile/gvint_block.h
index 6c386fd..d8d3042 100644
--- a/src/kudu/cfile/gvint_block.h
+++ b/src/kudu/cfile/gvint_block.h
@@ -43,7 +43,7 @@ using std::vector;
//
// See AppendGroupVarInt32(...) for details on the varint
// encoding.
-class GVIntBlockBuilder : public BlockBuilder {
+class GVIntBlockBuilder final : public BlockBuilder {
public:
explicit GVIntBlockBuilder(const WriterOptions *options);
@@ -98,7 +98,7 @@ class GVIntBlockBuilder : public BlockBuilder {
};
// Decoder for UINT32 type, GROUP_VARINT coding
-class GVIntBlockDecoder : public BlockDecoder {
+class GVIntBlockDecoder final : public BlockDecoder {
public:
explicit GVIntBlockDecoder(Slice slice);
http://git-wip-us.apache.org/repos/asf/kudu/blob/eb4b46b8/src/kudu/cfile/plain_bitmap_block.h
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/plain_bitmap_block.h
b/src/kudu/cfile/plain_bitmap_block.h
index 04e1f89..5414344 100644
--- a/src/kudu/cfile/plain_bitmap_block.h
+++ b/src/kudu/cfile/plain_bitmap_block.h
@@ -41,7 +41,7 @@ struct WriterOptions;
// A plain encoder for the BOOL datatype: stores a column of BOOL values
// as a packed bitmap.
//
-class PlainBitMapBlockBuilder : public BlockBuilder {
+class PlainBitMapBlockBuilder final : public BlockBuilder {
public:
explicit PlainBitMapBlockBuilder(const WriterOptions* options)
: writer_(&buf_),
@@ -106,7 +106,7 @@ class PlainBitMapBlockBuilder : public BlockBuilder {
//
// Plain decoder for the BOOL datatype
//
-class PlainBitMapBlockDecoder : public BlockDecoder {
+class PlainBitMapBlockDecoder final : public BlockDecoder {
public:
explicit PlainBitMapBlockDecoder(Slice slice)
: data_(std::move(slice)),
http://git-wip-us.apache.org/repos/asf/kudu/blob/eb4b46b8/src/kudu/cfile/plain_block.h
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/plain_block.h b/src/kudu/cfile/plain_block.h
index e01f3fa..4f9bcd6 100644
--- a/src/kudu/cfile/plain_block.h
+++ b/src/kudu/cfile/plain_block.h
@@ -43,7 +43,7 @@ static const size_t kPlainBlockHeaderSize = sizeof(uint32_t)
* 2;
// A plain encoder for generic fixed size data types.
//
template<DataType Type>
-class PlainBlockBuilder : public BlockBuilder {
+class PlainBlockBuilder final : public BlockBuilder {
public:
explicit PlainBlockBuilder(const WriterOptions *options)
: options_(options) {
@@ -109,7 +109,7 @@ class PlainBlockBuilder : public BlockBuilder {
// A plain decoder for generic fixed size data types.
//
template<DataType Type>
-class PlainBlockDecoder : public BlockDecoder {
+class PlainBlockDecoder final : public BlockDecoder {
public:
explicit PlainBlockDecoder(Slice slice)
: data_(std::move(slice)),
http://git-wip-us.apache.org/repos/asf/kudu/blob/eb4b46b8/src/kudu/cfile/rle_block.h
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/rle_block.h b/src/kudu/cfile/rle_block.h
index b441529..c7702f1 100644
--- a/src/kudu/cfile/rle_block.h
+++ b/src/kudu/cfile/rle_block.h
@@ -46,7 +46,7 @@ enum {
// RLE encoder for the BOOL datatype: uses an RLE-encoded bitmap to
// represent a bool column.
//
-class RleBitMapBlockBuilder : public BlockBuilder {
+class RleBitMapBlockBuilder final : public BlockBuilder {
public:
explicit RleBitMapBlockBuilder(const WriterOptions* options)
: encoder_(&buf_, 1),
@@ -107,7 +107,7 @@ class RleBitMapBlockBuilder : public BlockBuilder {
//
// RLE decoder for bool datatype
//
-class RleBitMapBlockDecoder : public BlockDecoder {
+class RleBitMapBlockDecoder final : public BlockDecoder {
public:
explicit RleBitMapBlockDecoder(Slice slice)
: data_(std::move(slice)),
@@ -213,7 +213,7 @@ class RleBitMapBlockDecoder : public BlockDecoder {
// TODO : consider if this can also be used for BOOL with only minor
// alterations
template <DataType IntType>
-class RleIntBlockBuilder : public BlockBuilder {
+class RleIntBlockBuilder final : public BlockBuilder {
public:
explicit RleIntBlockBuilder(const WriterOptions* options)
: rle_encoder_(&buf_, kCppTypeSize * 8),
@@ -296,7 +296,7 @@ class RleIntBlockBuilder : public BlockBuilder {
// code here for the BOOL type.
//
template <DataType IntType>
-class RleIntBlockDecoder : public BlockDecoder {
+class RleIntBlockDecoder final : public BlockDecoder {
public:
explicit RleIntBlockDecoder(Slice slice)
: data_(std::move(slice)),
