Merge pull request XRPLF#249 from tdfischer/decompression-refactoring

Decompression refactoring

table/block.cc

@@ -299,10 +299,7 @@ uint32_t Block::NumRestarts() const {
 
 Block::Block(const BlockContents& contents)
     : data_(contents.data.data()),
-      size_(contents.data.size()),
-      owned_(contents.heap_allocated),
-      cachable_(contents.cachable),
-      compression_type_(contents.compression_type) {
+      size_(contents.data.size()) {
   if (size_ < sizeof(uint32_t)) {
     size_ = 0;  // Error marker
   } else {
@@ -315,10 +312,8 @@ Block::Block(const BlockContents& contents)
   }
 }
 
-Block::~Block() {
-  if (owned_) {
-    delete[] data_;
-  }
+Block::Block(BlockContents&& contents) : Block(contents) {
+  contents_ = std::move(contents);
 }
 
 Iterator* Block::NewIterator(

table/block.h

@@ -14,6 +14,10 @@
 #include "rocksdb/iterator.h"
 #include "rocksdb/options.h"
 #include "db/dbformat.h"
+#include "table/block_prefix_index.h"
+#include "table/block_hash_index.h"
+
+#include "format.h"
 
 namespace rocksdb {
 
@@ -26,15 +30,16 @@ class BlockPrefixIndex;
 class Block {
  public:
   // Initialize the block with the specified contents.
+  explicit Block(BlockContents&& contents);
   explicit Block(const BlockContents& contents);
 
-  ~Block();
+  ~Block() = default;
 
   size_t size() const { return size_; }
   const char* data() const { return data_; }
-  bool cachable() const { return cachable_; }
+  bool cachable() const { return contents_.cachable; }
   uint32_t NumRestarts() const;
-  CompressionType compression_type() const { return compression_type_; }
+  CompressionType compression_type() const { return contents_.compression_type; }
 
   // If hash index lookup is enabled and `use_hash_index` is true. This block
   // will do hash lookup for the key prefix.
@@ -58,12 +63,10 @@ class Block {
   size_t ApproximateMemoryUsage() const;
 
  private:
+  BlockContents contents_;
   const char* data_;
   size_t size_;
   uint32_t restart_offset_;     // Offset in data_ of restart array
-  bool owned_;                  // Block owns data_[]
-  bool cachable_;
-  CompressionType compression_type_;
   std::unique_ptr<BlockHashIndex> hash_index_;
   std::unique_ptr<BlockPrefixIndex> prefix_index_;
 

table/block_based_filter_block.cc

@@ -138,7 +138,7 @@ void BlockBasedFilterBlockBuilder::GenerateFilter() {
 BlockBasedFilterBlockReader::BlockBasedFilterBlockReader(
     const SliceTransform* prefix_extractor,
     const BlockBasedTableOptions& table_opt,
-    const Slice& contents, bool delete_contents_after_use)
+    const Slice& contents)
     : policy_(table_opt.filter_policy.get()),
       prefix_extractor_(prefix_extractor),
       whole_key_filtering_(table_opt.whole_key_filtering),
@@ -155,9 +155,14 @@ BlockBasedFilterBlockReader::BlockBasedFilterBlockReader(
   data_ = contents.data();
   offset_ = data_ + last_word;
   num_ = (n - 5 - last_word) / 4;
-  if (delete_contents_after_use) {
-    filter_data.reset(contents.data());
-  }
+}
+
+BlockBasedFilterBlockReader::BlockBasedFilterBlockReader(
+    const SliceTransform* prefix_extractor,
+    const BlockBasedTableOptions& table_opt,
+    BlockContents &&contents)
+    : BlockBasedFilterBlockReader (prefix_extractor, table_opt, contents.data) {
+  contents_ = std::move(contents);
 }
 
 bool BlockBasedFilterBlockReader::KeyMayMatch(const Slice& key,

table/block_based_filter_block.h

@@ -74,8 +74,10 @@ class BlockBasedFilterBlockReader : public FilterBlockReader {
   // REQUIRES: "contents" and *policy must stay live while *this is live.
   BlockBasedFilterBlockReader(const SliceTransform* prefix_extractor,
                               const BlockBasedTableOptions& table_opt,
-                              const Slice& contents,
-                              bool delete_contents_after_use = false);
+                              const Slice& contents);
+  BlockBasedFilterBlockReader(const SliceTransform* prefix_extractor,
+                              const BlockBasedTableOptions& table_opt,
+                              BlockContents&& contents);
   virtual bool IsBlockBased() override { return true; }
   virtual bool KeyMayMatch(const Slice& key,
                            uint64_t block_offset = kNotValid) override;
@@ -91,7 +93,7 @@ class BlockBasedFilterBlockReader : public FilterBlockReader {
   const char* offset_;  // Pointer to beginning of offset array (at block-end)
   size_t num_;          // Number of entries in offset array
   size_t base_lg_;      // Encoding parameter (see kFilterBaseLg in .cc file)
-  std::unique_ptr<const char[]> filter_data;
+  BlockContents contents_;
 
   bool MayMatch(const Slice& entry, uint64_t block_offset);
 

table/block_based_table_builder.cc

@@ -17,6 +17,7 @@
 #include <memory>
 #include <string>
 #include <unordered_map>
+#include <utility>
 
 #include "db/dbformat.h"
 
@@ -634,18 +635,13 @@ Status BlockBasedTableBuilder::InsertBlockInCache(const Slice& block_contents,
     Cache::Handle* cache_handle = nullptr;
     size_t size = block_contents.size();
 
-    char* ubuf = new char[size + 1];  // make a new copy
-    memcpy(ubuf, block_contents.data(), size);
+    std::unique_ptr<char[]> ubuf(new char[size+1]);
+    memcpy(ubuf.get(), block_contents.data(), size);
     ubuf[size] = type;
 
-    BlockContents results;
-    Slice sl(ubuf, size);
-    results.data = sl;
-    results.cachable = true; // XXX
-    results.heap_allocated = true;
-    results.compression_type = type;
+    BlockContents results(std::move(ubuf), size, true, type);
 
-    Block* block = new Block(results);
+    Block* block = new Block(std::move(results));
 
     // make cache key by appending the file offset to the cache prefix id
     char* end = EncodeVarint64(

table/block_based_table_reader.cc

@@ -66,7 +66,7 @@ Status ReadBlockFromFile(RandomAccessFile* file, const Footer& footer,
   Status s = ReadBlockContents(file, footer, options, handle, &contents, env,
                                do_uncompress);
   if (s.ok()) {
-    *result = new Block(contents);
+    *result = new Block(std::move(contents));
   }
 
   return s;
@@ -252,9 +252,6 @@ class HashIndexReader : public IndexReader {
                           &prefixes_meta_contents, env,
                           true /* do decompression */);
     if (!s.ok()) {
-      if (prefixes_contents.heap_allocated) {
-        delete[] prefixes_contents.data.data();
-      }
       // TODO: log error
       return Status::OK();
     }
@@ -269,7 +266,7 @@ class HashIndexReader : public IndexReader {
       // TODO: log error
       if (s.ok()) {
         new_index_reader->index_block_->SetBlockHashIndex(hash_index);
-        new_index_reader->OwnPrefixesContents(prefixes_contents);
+        new_index_reader->OwnPrefixesContents(std::move(prefixes_contents));
       }
     } else {
       BlockPrefixIndex* prefix_index = nullptr;
@@ -283,18 +280,6 @@ class HashIndexReader : public IndexReader {
       }
     }
 
-    // Always release prefix meta block
-    if (prefixes_meta_contents.heap_allocated) {
-      delete[] prefixes_meta_contents.data.data();
-    }
-
-    // Release prefix content block if we don't own it.
-    if (!new_index_reader->own_prefixes_contents_) {
-      if (prefixes_contents.heap_allocated) {
-        delete[] prefixes_contents.data.data();
-      }
-    }
-
     return Status::OK();
   }
 
@@ -314,24 +299,18 @@ class HashIndexReader : public IndexReader {
  private:
   HashIndexReader(const Comparator* comparator, Block* index_block)
       : IndexReader(comparator),
-        index_block_(index_block),
-        own_prefixes_contents_(false) {
+        index_block_(index_block) {
     assert(index_block_ != nullptr);
   }
 
   ~HashIndexReader() {
-    if (own_prefixes_contents_ && prefixes_contents_.heap_allocated) {
-      delete[] prefixes_contents_.data.data();
-    }
   }
 
-  void OwnPrefixesContents(const BlockContents& prefixes_contents) {
-    prefixes_contents_ = prefixes_contents;
-    own_prefixes_contents_ = true;
+  void OwnPrefixesContents(BlockContents&& prefixes_contents) {
+    prefixes_contents_ = std::move(prefixes_contents);
   }
 
   std::unique_ptr<Block> index_block_;
-  bool own_prefixes_contents_;
   BlockContents prefixes_contents_;
 };
 
@@ -677,7 +656,7 @@ Status BlockBasedTable::GetDataBlockFromCache(
 
   // Insert uncompressed block into block cache
   if (s.ok()) {
-    block->value = new Block(contents);  // uncompressed block
+    block->value = new Block(std::move(contents));  // uncompressed block
     assert(block->value->compression_type() == kNoCompression);
     if (block_cache != nullptr && block->value->cachable() &&
         read_options.fill_cache) {
@@ -715,7 +694,7 @@ Status BlockBasedTable::PutDataBlockToCache(
   }
 
   if (raw_block->compression_type() != kNoCompression) {
-    block->value = new Block(contents);  // uncompressed block
+    block->value = new Block(std::move(contents));  // uncompressed block
   } else {
     block->value = raw_block;
     raw_block = nullptr;
@@ -768,15 +747,14 @@ FilterBlockReader* BlockBasedTable::ReadFilter(
   assert(rep->filter_policy);
   if (kFilterBlockPrefix == filter_block_prefix) {
     return new BlockBasedFilterBlockReader(rep->ioptions.prefix_extractor,
-        rep->table_options, block.data, block.heap_allocated);
+        rep->table_options, std::move(block));
   } else if (kFullFilterBlockPrefix == filter_block_prefix) {
     auto filter_bits_reader = rep->filter_policy->
         GetFilterBitsReader(block.data);
 
     if (filter_bits_reader != nullptr) {
       return new FullFilterBlockReader(rep->ioptions.prefix_extractor,
-          rep->table_options, block.data, filter_bits_reader,
-          block.heap_allocated);
+          rep->table_options, std::move(block), filter_bits_reader);
     }
   }
   return nullptr;

table/block_test.cc

@@ -92,8 +92,7 @@ TEST(BlockTest, SimpleTest) {
   BlockContents contents;
   contents.data = rawblock;
   contents.cachable = false;
-  contents.heap_allocated = false;
-  Block reader(contents);
+  Block reader(std::move(contents));
 
   // read contents of block sequentially
   int count = 0;
@@ -143,12 +142,11 @@ BlockContents GetBlockContents(std::unique_ptr<BlockBuilder> *builder,
   BlockContents contents;
   contents.data = rawblock;
   contents.cachable = false;
-  contents.heap_allocated = false;
 
   return contents;
 }
 
-void CheckBlockContents(BlockContents contents, const int max_key,
+void CheckBlockContents(const BlockContents &contents, const int max_key,
                         const std::vector<std::string> &keys,
                         const std::vector<std::string> &values) {
   const size_t prefix_size = 6;

table/filter_block.h

@@ -18,17 +18,22 @@
 
 #pragma once
 
+#include <memory>
 #include <stddef.h>
 #include <stdint.h>
 #include <string>
 #include <vector>
 #include "rocksdb/options.h"
 #include "rocksdb/slice.h"
+#include "rocksdb/slice_transform.h"
 #include "rocksdb/table.h"
+#include "util/hash.h"
+#include "format.h"
 
 namespace rocksdb {
 
 const uint64_t kNotValid = ULLONG_MAX;
+class FilterPolicy;
 
 // A FilterBlockBuilder is used to construct all of the filters for a
 // particular Table.  It generates a single string which is stored as