Add Compressor interface #584

lucagiac81 · 2023-06-26T22:22:38Z

This PR refactors compression by introducing a Compressor interface.
This is a first step towards supporting compressors as plugins. PR #585 covers the next step.
It is equivalent to PR7650 in RocksDB.

Compressor class

The Compressor class defines an interface for each compression algorithm to implement. It is a Customizable class, like other extensible components in Speedb.

A Compressor has

A unique name
Compress/Uncompress methods
CreateDictionary method (for algorithms supporting dictionaries): the logic to build/train a dictionary is moved here, so future compressors have the option to customize it if necessary
Methods to handle processed/digested dictionaries (implemented by zstd, for example)
Options: each Compressor can define the options it supports using the Configurable framework

Built-in compressors

The existing compression algorithms (referred to as "built-in") are encapsulated in Compressor classes. The classes inherit from BuiltinCompressor, which includes functionality shared by all built-in compressors.
Built-in compressors can be referenced by their numeric id (as defined by the CompressionType enum) to ensure backward compatibility. BuiltinCompressor uses the existing CompressionOptions struct as its configurable options.

Compressor lifecycle

For this PR, compression options exposed in the public API are unchanged (exposing Compressor in the public API and options is the scope of PR #585).

The CompressionType and CompressionOptions passed through ColumnFamilyOptions and DBOptions are used to instantiate suitable Compressor instances (this is done in MutableCFOptions and ImmutableDBOptions).
The Compressor class keeps track of the instances currently in use, so they can be retrieved and reused.
Such Compressor instances are then used in other parts of the code in place of CompressionType and CompressionOptions (e.g., in BlockBasedTableBuilder, BlockBasedTable, FlushJob, Compaction, BlobFileBuilder...).
The details of the Compressor used for a block-based table are serialized in the Properties block.
When opening the SST file, the info in the Properties block is used to instantiate/retrieve a suitable Compressor for the table. If the compression id for a block doesn't match the table-level Compressor, a suitable Compressor is obtained when reading the block.

TODO

Streaming compression is not included in the Compressor class yet. The plan is to cover that in a separate PR.
It could be offered through additional methods in the Compressor class. For example

CreateStreamingCompressContext(): returns a context object similar to StreamingCompress
StreamingCompress(context,...): compress method takes context and the usual input/output buffers
Similar methods for uncompress
StreamingSupported(): to query capability of a Compressor

mrambacher

Need to finish util/compressor class but have done the rest. Looks good so far!

mrambacher · 2023-07-04T00:09:35Z

db/blob/blob_file_builder.cc

@@ -150,7 +150,7 @@ Status BlobFileBuilder::Add(const Slice& key, const Slice& value,
  }

  BlobIndex::EncodeBlob(blob_index, blob_file_number, blob_offset, blob.size(),
-                        blob_compression_type_);
+                        blob_compressor_->GetCompressionType());


Looking through the code, I am not sure if blob_compressor_ is always set. Do you know if that is true? Should there be an assert somewhere?

blob_compressor_ is initialized in BlobFileBuilder's constructor from mutable_cf_options.blob_compressor. The default constructor of MutableCFOptions sets blob_compressor_ to nullptr.
I'm not sure if we can ever run into a situation where blob_compressor_ is left null, but I'll initialize it to NoCompressor in the BlobFileBuilder's constructor to be safe. This better aligns with the previous code initializing to kNoCompression by default.

mrambacher · 2023-07-04T00:37:37Z

db/compaction/compaction.cc

-    bool _manual_compaction, const std::string& _trim_ts, double _score,
-    bool _deletion_compaction, bool l0_files_might_overlap,
-    CompactionReason _compaction_reason,
+    uint32_t _output_path_id, std::shared_ptr<Compressor> _compressor,


const std::shared_ptr& ?

Fixed in next push

mrambacher · 2023-07-04T00:48:54Z

db/db_options_test.cc

@@ -1150,7 +1149,6 @@ TEST_F(DBOptionsTest, ChangeCompression) {
  ASSERT_OK(dbfull()->TEST_WaitForCompact());
  ASSERT_TRUE(compacted);
  ASSERT_EQ(CompressionType::kSnappyCompression, compression_used);
-  ASSERT_EQ(6, compression_opt_used.level);


Is there a reason this line was removed?

The assertion was added again in the second PR (with some changes). I'm moving it back here.

mrambacher · 2023-07-04T00:55:24Z

env/env.cc

@@ -26,6 +26,7 @@
 #include "rocksdb/utilities/object_registry.h"
 #include "rocksdb/utilities/options_type.h"
 #include "util/autovector.h"
+#include "util/string_util.h"


Is this required?

I verified that it is. Env::GenerateUniqueId calls PutBaseChars in string_util.h. Include changes in other files led to this.

mrambacher · 2023-07-04T01:04:30Z

table/block_based/block_based_table_builder.cc


-  // Abstract algebra teaches us that a finite cyclic group (such as the


Where did this code go? Was it moved or is it no longer necessary?

It moved to util/compressor.cc. It is part of the default implementation to sample and create a dictionary. The Compressor interface allows plugins to override the methods and customize dictionary creation if desired.

mrambacher · 2023-07-04T01:05:31Z

table/block_based/block_based_table_reader.cc

@@ -142,15 +142,16 @@ Status ReadAndParseBlockFromFile(
    BlockCreateContext& create_context, bool maybe_compressed,
    const UncompressionDict& uncompression_dict,
    const PersistentCacheOptions& cache_options,
-    MemoryAllocator* memory_allocator, bool for_compaction, bool async_read) {
+    MemoryAllocator* memory_allocator, bool for_compaction,
+    std::shared_ptr<Compressor>& compressor, bool async_read) {


///should this be a const?

Yes, it should. Fixed.

mrambacher · 2023-07-04T01:07:38Z

table/block_based/block_based_table_reader.cc

+                                       &rep_->compressor);
+      if (!s.ok() || rep_->compressor == nullptr) {
+        ROCKS_LOG_ERROR(rep_->ioptions.logger,
+                        "Compression type not supported");


Should this also set "blocks_maybe_compressed=false"?

Compressor::CreateFromString failing means that the compressor needed by this table is missing. Instantiating NoCompressor (condition for blocks_maybe_compressed=false) should never fail. This scenario should eventually lead to decompression failing.

mrambacher · 2023-07-04T01:14:27Z

table/table_builder.h

@@ -93,8 +93,7 @@ struct TableBuilderOptions {
      const ImmutableOptions& _ioptions, const MutableCFOptions& _moptions,
      const InternalKeyComparator& _internal_comparator,
      const IntTblPropCollectorFactories* _int_tbl_prop_collector_factories,
-      CompressionType _compression_type,
-      const CompressionOptions& _compression_opts, uint32_t _column_family_id,
+      std::shared_ptr<Compressor> _compressor, uint32_t _column_family_id,


mrambacher · 2023-07-04T01:19:07Z

utilities/blob_db/blob_dump_tool.cc

@@ -56,8 +56,8 @@ Status BlobDumpTool::Run(const std::string& filename, DisplayType show_key,
  reader_.reset(new RandomAccessFileReader(std::move(file), filename));
  uint64_t offset = 0;
  uint64_t footer_offset = 0;
-  CompressionType compression = kNoCompression;
-  s = DumpBlobLogHeader(&offset, &compression);
+  std::shared_ptr<Compressor> compressor;


Should this be null or a NoCompression compressor?

compressor is initialized in DumpBlobLogHeader. It takes compression from BlobLogHeader (kNoCompression by default).

mrambacher · 2023-07-17T15:01:52Z

util/compression.cc

@@ -1,11 +1,684 @@
-// Copyright (c) 2022-present, Facebook, Inc.  All rights reserved.
+// Copyright (c) 2011-present, Facebook, Inc.  All rights reserved.


The year here changed...

Fixed. We introduced compression.cc first in the PRs, but it got later created in RocksDB too, and the copyright date change was missed.

mrambacher · 2023-07-17T17:25:26Z

util/compression.cc

+      return true;
+    }
+  }
+  return false;


Should type be set to Unknown/None or something on error (similar to how result is cleared)?

If StringToType returns false, the caller should not rely on the value of type. All uses of StringToType adhere to this expectation (except for a few tests, where the string is known to be a valid name).

mrambacher · 2023-07-17T17:31:30Z

util/compression.h

@@ -666,55 +282,66 @@ inline std::string CompressionOptionsToString(
  return result;
 }

+#ifdef SNAPPY


Are the XXX_Compress/Uncompress functions still needed in a header file or can they be local to the source file now? If they cannot be moved to the source file, can you please add a TODO comment to make that move when it is available?

The functions were left in the header file for ease of review. They should be moved to the source file eventually. Adding a TODO comment.

mrambacher · 2023-07-17T17:36:56Z

util/compressor.h

+
+  // Returns a new uncompression dictionary from the input dict.
+  // Classes which have a ProcessedDict should override this method.
+  virtual UncompressionDict* NewUncompressionDict(const std::string& dict) {


Should these return a std::unique_ptr? Maybe something that should happen in the next phase?

The pointer returned by NewUncompressionDict is used to reset a unique_ptr in the two usages I found in the code (BlockCreateContext::Create and BlockBasedTableBuilder::EnterUnbuffered). This is similar to the previous code, where the new operator was used.

mrambacher · 2023-07-17T17:40:25Z

util/compressor.h

+      const std::string& compression_dict_samples,
+      const std::vector<size_t>& compression_dict_sample_lens);
+
+ private:


Personally I would rather these private functions were not in the interface if it can be avoided. Can you explain when/where/why they are used and if there is a way to move them to something local?

These private getters are used to access certain compression options: GetMaxDictBytes, GetMaxTrainBytes, UseDictTrainer, GetLevel. These options are used by some Compressor methods with default implementations (e.g., SampleDict and TrainDict). Different compressors could customize the names of the underlying options, and the getters provide a standard way to access them.
For example, SampleDict and TrainDict provide the default RocksDB dictionary creation logic. Compressors are free to add their custom logic, but I'd expect many compressors to rely on the default implementation.

RocksDB code outside Compressor does not need these options. There are other similar option getters that are public, as other RocksDB code needs to access them (GetMaxDictBufferBytes, GetParallelThreads).

Making these getters private may be a bit too restrictive. We could at least make them protected.

mrambacher · 2023-07-17T17:46:24Z

util/compressor.cc

+void Compressor::SampleDict(std::vector<std::string>& data_block_buffers,
+                            std::string* compression_dict_samples,
+                            std::vector<size_t>* compression_dict_sample_lens) {


Should these be a static method or somehow otherwise denoted? It does not appear to use anything inside of the Compressor (other than dictionary sizes).

SampleDict and TrainDict provide the default RocksDB dictionary creation logic. These methods are used in the default implementation of CreateDict. I split the two phases of dictionary creation as Compressors may want to use to customize only sampling or training (I ran into that scenario). Compressors are free to customize any of these methods if they need custom logic for one or both phases.

An alternative (also covering the private getters in the previous comment) would be to place the default dictionary creation code in a utility class. Compressors would then use the utility class in their CreateDict method to use the default logic.

mrambacher · 2023-07-17T17:47:21Z

util/compressor.cc

+      dict = ZSTD_TrainDictionary(compression_dict_samples,
+                                  compression_dict_sample_lens, max_dict_bytes);
+    } else {
+      dict = ZSTD_FinalizeDictionary(compression_dict_samples,
+                                     compression_dict_sample_lens,
+                                     max_dict_bytes, level);
+    }


Is the dictionary training the same for all dictionaries and not dependent on ZSTD?

What happens if built without ZSTD enabled?

The ZSTD dictionary trainer is used with other compression algorithms too.

ZSTD_TrainDictionary has conditional compilation based on ZSTD library version. If the trainer is not supported, it returns an empty string.

There is also a ZSTD_TrainDictionarySupported function with the same library version check. It is used in CheckCompressionSupported. That only checks CompressionOptions, not Compressor options (as a Compressor could define a custom trainer).

Similar comments apply to ZSTD_FinalizeDictionary.

I added a check for ZSTD_TrainDictionarySupported in TrainDict's default implementation. If the Compressor selects to use a trainer (based on GetMaxTrainBytes and UseDictTrainer), it checks if ZSTD_TrainDictionary is supported. If not, it checks if ZSTD_FinalizeDictionary is supported. If neither is supported, it will return back the original samples.

mrambacher

Looks great! Thanks for the PR

ofriedma · 2023-10-18T10:37:55Z

@lucagiac81 please rebase

udi-speedb · 2023-10-29T09:42:16Z

@ofriedma, @speedbmike - Please review

lucagiac81 · 2024-01-11T16:08:57Z

Code is rebased on latest main based RocksDB 8.6.7 (the previous code was based on RocksDB 8.1.1).

One note regarding ZSTDContext

ZSTDContext replaces CompressionContext, making it Compressor-specific. A Compressor can manage its own context as needed (only ZSTD currently needs this).
In the previous version of the PR, there was one ZSTDContext instance per thread.
After RocksDB PR 11666, ZSTDContext depends on the options selected for ZSTDCompressor. So, different ZSTDCompressor instances (with potentially different options) should not share ZSTDContext instances.
The latest code has one ZSTDContext instance per thread, but internally a context is created for each instance of ZSTDCompressor. The net effect is to have one context instance per thread per ZSTDCompressor instance.

CLAassistant · 2024-02-01T08:25:30Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

lucagiac81 mentioned this pull request Jun 26, 2023

Add support for compressor plugins #585

Open

lucagiac81 marked this pull request as ready for review June 27, 2023 14:06

mrambacher reviewed Jul 4, 2023

View reviewed changes

mrambacher reviewed Jul 17, 2023

View reviewed changes

lucagiac81 force-pushed the compressor_interface branch from 241d920 to 1def930 Compare August 15, 2023 20:06

mrambacher previously approved these changes Sep 18, 2023

View reviewed changes

lucagiac81 force-pushed the compressor_interface branch from 1def930 to 439bc30 Compare October 19, 2023 18:06

lucagiac81 dismissed mrambacher’s stale review via 439bc30 October 22, 2023 08:51

udi-speedb requested review from ofriedma and speedbmike October 29, 2023 09:43

Guyme assigned ofriedma Nov 2, 2023

Yuval-Ariel force-pushed the main branch from 87d2f42 to 5e356db Compare December 25, 2023 09:40

Add Compressor interface

d88100c

lucagiac81 force-pushed the compressor_interface branch from 439bc30 to d88100c Compare January 11, 2024 16:06


		// Abstract algebra teaches us that a finite cyclic group (such as the

		@@ -1,11 +1,684 @@
		// Copyright (c) 2022-present, Facebook, Inc. All rights reserved.
		// Copyright (c) 2011-present, Facebook, Inc. All rights reserved.

Add Compressor interface #584

Are you sure you want to change the base?

Add Compressor interface #584

Conversation

lucagiac81 commented Jun 26, 2023 • edited

Compressor class

Built-in compressors

Compressor lifecycle

TODO

mrambacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucagiac81 Aug 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrambacher left a comment

Choose a reason for hiding this comment

ofriedma commented Oct 18, 2023

udi-speedb commented Oct 29, 2023

lucagiac81 commented Jan 11, 2024 • edited

CLAassistant commented Feb 1, 2024

lucagiac81 commented Jun 26, 2023 •

edited

lucagiac81 Aug 9, 2023 •

edited

lucagiac81 commented Jan 11, 2024 •

edited