Skip to content

Commit

Permalink
Require ZSTD >= 1.4.0, greatly simplifying related code
Browse files Browse the repository at this point in the history
Summary: Leading up to some compression code refactoring, we have a bit
of an ifdef nightmare in compression.h relating to zstd support. With
the major release RocksDB 10.0.0 coming up, it is a good time to clean
up much of this tech debt by requiring zstd >= 1.4.0 (April 2019) if
building RocksDB with ZSTD support.
* Most of the `ZSTD_VERSION_NUMBER` checks are simplified to just
  `ZSTD`, though
  * `ROCKSDB_ZSTD_DDICT` still needs to be separate because of
    dependency on `ZSTD_STATIC_LINKING_ONLY` (added to
    fbcode_config_platform010.sh by the way)
* Eliminate deprecated `kZSTDNotFinalCompression`
* Reduce some cases of unnecessary copying definitions across `#if`
  branches (e.g. `ZSTDUncompressCachedData`)

Test Plan: minor unit test updates. `make check` on several build
variants with/without zstd and with/without `ZSTD_STATIC_LINKING_ONLY`
  • Loading branch information
pdillinger committed Jan 31, 2025
1 parent 57c177b commit 991a239
Show file tree
Hide file tree
Showing 12 changed files with 147 additions and 262 deletions.
6 changes: 5 additions & 1 deletion build_tools/build_detect_platform
Original file line number Diff line number Diff line change
Expand Up @@ -360,9 +360,13 @@ EOF
fi

if ! test $ROCKSDB_DISABLE_ZSTD; then
# Test whether zstd library is installed
# Test whether zstd library is installed with minimum version
# (Keep in sync with compression.h)
$CXX $PLATFORM_CXXFLAGS $COMMON_FLAGS -x c++ - -o /dev/null 2>/dev/null <<EOF
#include <zstd.h>
#if ZSTD_VERSION_NUMBER < 10400
#error "ZSTD support requires version >= 1.4.0 (libzstd-devel)"
#endif // ZSTD_VERSION_NUMBER
int main() {}
EOF
if [ "$?" = 0 ]; then
Expand Down
2 changes: 1 addition & 1 deletion build_tools/fbcode_config_platform010.sh
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ fi
if ! test $ROCKSDB_DISABLE_ZSTD; then
ZSTD_INCLUDE=" -I $ZSTD_BASE/include/"
ZSTD_LIBS=" $ZSTD_BASE/lib/libzstd${MAYBE_PIC}.a"
CFLAGS+=" -DZSTD"
CFLAGS+=" -DZSTD -DZSTD_STATIC_LINKING_ONLY"
fi

# location of gflags headers and libraries
Expand Down
2 changes: 0 additions & 2 deletions db/db_block_cache_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -835,8 +835,6 @@ TEST_F(DBBlockCacheTest, CacheCompressionDict) {
}
if (ZSTD_Supported()) {
compression_types.push_back(kZSTD);
} else if (ZSTDNotFinal_Supported()) {
compression_types.push_back(kZSTDNotFinalCompression);
}
Random rnd(301);
for (auto compression_type : compression_types) {
Expand Down
11 changes: 4 additions & 7 deletions db/db_test2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1475,8 +1475,7 @@ TEST_P(PresetCompressionDictTest, Flush) {
// TODO(ajkr): fix the below assertion to work with ZSTD. The expectation on
// number of bytes needs to be adjusted in case the cached block is in
// ZSTD's digested dictionary format.
if (compression_type_ != kZSTD &&
compression_type_ != kZSTDNotFinalCompression) {
if (compression_type_ != kZSTD) {
// Although we limited buffering to `kBlockLen`, there may be up to two
// blocks of data included in the dictionary since we only check limit
// after each block is built.
Expand Down Expand Up @@ -1553,8 +1552,7 @@ TEST_P(PresetCompressionDictTest, CompactNonBottommost) {
// TODO(ajkr): fix the below assertion to work with ZSTD. The expectation on
// number of bytes needs to be adjusted in case the cached block is in
// ZSTD's digested dictionary format.
if (compression_type_ != kZSTD &&
compression_type_ != kZSTDNotFinalCompression) {
if (compression_type_ != kZSTD) {
// Although we limited buffering to `kBlockLen`, there may be up to two
// blocks of data included in the dictionary since we only check limit
// after each block is built.
Expand Down Expand Up @@ -1615,8 +1613,7 @@ TEST_P(PresetCompressionDictTest, CompactBottommost) {
// TODO(ajkr): fix the below assertion to work with ZSTD. The expectation on
// number of bytes needs to be adjusted in case the cached block is in ZSTD's
// digested dictionary format.
if (compression_type_ != kZSTD &&
compression_type_ != kZSTDNotFinalCompression) {
if (compression_type_ != kZSTD) {
// Although we limited buffering to `kBlockLen`, there may be up to two
// blocks of data included in the dictionary since we only check limit after
// each block is built.
Expand Down Expand Up @@ -8000,7 +7997,7 @@ TEST_F(DBTest2, GetLatestSeqAndTsForKey) {
ASSERT_EQ(0, options.statistics->getTickerCount(GET_HIT_L0));
}

#if defined(ZSTD_ADVANCED)
#if defined(ZSTD)
TEST_F(DBTest2, ZSTDChecksum) {
// Verify that corruption during decompression is caught.
Options options = CurrentOptions();
Expand Down
7 changes: 0 additions & 7 deletions include/rocksdb/compression_type.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,6 @@ enum CompressionType : unsigned char {
kXpressCompression = 0x6,
kZSTD = 0x7,

// Only use kZSTDNotFinalCompression if you have to use ZSTD lib older than
// 0.8.0 or consider a possibility of downgrading the service or copying
// the database files to another service running with an older version of
// RocksDB that doesn't have kZSTD. Otherwise, you should use kZSTD. We will
// eventually remove the option from the public API.
kZSTDNotFinalCompression = 0x40,

// kDisableCompressionOption is used to disable some compression options.
kDisableCompressionOption = 0xff,
};
Expand Down
1 change: 0 additions & 1 deletion options/options_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,6 @@ std::unordered_map<std::string, CompressionType>
{"kLZ4HCCompression", kLZ4HCCompression},
{"kXpressCompression", kXpressCompression},
{"kZSTD", kZSTD},
{"kZSTDNotFinalCompression", kZSTDNotFinalCompression},
{"kDisableCompressionOption", kDisableCompressionOption}};

std::vector<CompressionType> GetSupportedCompressions() {
Expand Down
12 changes: 4 additions & 8 deletions options/options_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,7 @@ TEST_F(OptionsTest, GetOptionsFromMapTest) {
"kLZ4Compression:"
"kLZ4HCCompression:"
"kXpressCompression:"
"kZSTD:"
"kZSTDNotFinalCompression"},
"kZSTD"},
{"bottommost_compression", "kLZ4Compression"},
{"bottommost_compression_opts", "5:6:7:8:10:true"},
{"compression_opts", "4:5:6:7:8:2:true:100:false"},
Expand Down Expand Up @@ -204,7 +203,7 @@ TEST_F(OptionsTest, GetOptionsFromMapTest) {
ASSERT_EQ(new_cf_opt.max_write_buffer_number_to_maintain, 99);
ASSERT_EQ(new_cf_opt.max_write_buffer_size_to_maintain, -99999);
ASSERT_EQ(new_cf_opt.compression, kSnappyCompression);
ASSERT_EQ(new_cf_opt.compression_per_level.size(), 9U);
ASSERT_EQ(new_cf_opt.compression_per_level.size(), 8U);
ASSERT_EQ(new_cf_opt.compression_per_level[0], kNoCompression);
ASSERT_EQ(new_cf_opt.compression_per_level[1], kSnappyCompression);
ASSERT_EQ(new_cf_opt.compression_per_level[2], kZlibCompression);
Expand All @@ -213,7 +212,6 @@ TEST_F(OptionsTest, GetOptionsFromMapTest) {
ASSERT_EQ(new_cf_opt.compression_per_level[5], kLZ4HCCompression);
ASSERT_EQ(new_cf_opt.compression_per_level[6], kXpressCompression);
ASSERT_EQ(new_cf_opt.compression_per_level[7], kZSTD);
ASSERT_EQ(new_cf_opt.compression_per_level[8], kZSTDNotFinalCompression);
ASSERT_EQ(new_cf_opt.compression_opts.window_bits, 4);
ASSERT_EQ(new_cf_opt.compression_opts.level, 5);
ASSERT_EQ(new_cf_opt.compression_opts.strategy, 6);
Expand Down Expand Up @@ -2384,8 +2382,7 @@ TEST_F(OptionsOldApiTest, GetOptionsFromMapTest) {
"kLZ4Compression:"
"kLZ4HCCompression:"
"kXpressCompression:"
"kZSTD:"
"kZSTDNotFinalCompression"},
"kZSTD"},
{"bottommost_compression", "kLZ4Compression"},
{"bottommost_compression_opts", "5:6:7:8:9:true"},
{"compression_opts", "4:5:6:7:8:9:true:10:false"},
Expand Down Expand Up @@ -2504,7 +2501,7 @@ TEST_F(OptionsOldApiTest, GetOptionsFromMapTest) {
ASSERT_EQ(new_cf_opt.max_write_buffer_number_to_maintain, 99);
ASSERT_EQ(new_cf_opt.max_write_buffer_size_to_maintain, -99999);
ASSERT_EQ(new_cf_opt.compression, kSnappyCompression);
ASSERT_EQ(new_cf_opt.compression_per_level.size(), 9U);
ASSERT_EQ(new_cf_opt.compression_per_level.size(), 8U);
ASSERT_EQ(new_cf_opt.compression_per_level[0], kNoCompression);
ASSERT_EQ(new_cf_opt.compression_per_level[1], kSnappyCompression);
ASSERT_EQ(new_cf_opt.compression_per_level[2], kZlibCompression);
Expand All @@ -2513,7 +2510,6 @@ TEST_F(OptionsOldApiTest, GetOptionsFromMapTest) {
ASSERT_EQ(new_cf_opt.compression_per_level[5], kLZ4HCCompression);
ASSERT_EQ(new_cf_opt.compression_per_level[6], kXpressCompression);
ASSERT_EQ(new_cf_opt.compression_per_level[7], kZSTD);
ASSERT_EQ(new_cf_opt.compression_per_level[8], kZSTDNotFinalCompression);
ASSERT_EQ(new_cf_opt.compression_opts.window_bits, 4);
ASSERT_EQ(new_cf_opt.compression_opts.level, 5);
ASSERT_EQ(new_cf_opt.compression_opts.strategy, 6);
Expand Down
8 changes: 3 additions & 5 deletions table/block_based/block_based_table_builder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -500,8 +500,7 @@ struct BlockBasedTableBuilder::Rep {
table_options.flush_block_policy_factory->NewFlushBlockPolicy(
table_options, data_block)),
create_context(&table_options, &ioptions, ioptions.stats,
compression_type == kZSTD ||
compression_type == kZSTDNotFinalCompression,
compression_type == kZSTD,
tbo.moptions.block_protection_bytes_per_key,
tbo.internal_comparator.user_comparator(),
!use_delta_encoding_for_index_values,
Expand Down Expand Up @@ -1958,9 +1957,8 @@ void BlockBasedTableBuilder::EnterUnbuffered() {
}
r->compression_dict.reset(new CompressionDict(dict, r->compression_type,
r->compression_opts.level));
r->verify_dict.reset(new UncompressionDict(
dict, r->compression_type == kZSTD ||
r->compression_type == kZSTDNotFinalCompression));
r->verify_dict.reset(
new UncompressionDict(dict, r->compression_type == kZSTD));

auto get_iterator_for_block = [&r](size_t i) {
auto& data_block = r->data_block_buffers[i];
Expand Down
7 changes: 2 additions & 5 deletions table/block_based/block_based_table_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -740,11 +740,8 @@ Status BlockBasedTable::Open(

// Populate BlockCreateContext
bool blocks_definitely_zstd_compressed =
rep->table_properties &&
(rep->table_properties->compression_name ==
CompressionTypeToString(kZSTD) ||
rep->table_properties->compression_name ==
CompressionTypeToString(kZSTDNotFinalCompression));
rep->table_properties && (rep->table_properties->compression_name ==
CompressionTypeToString(kZSTD));
rep->create_context = BlockCreateContext(
&rep->table_options, &rep->ioptions, rep->ioptions.stats,
blocks_definitely_zstd_compressed, block_protection_bytes_per_key,
Expand Down
1 change: 1 addition & 0 deletions unreleased_history/public_api_changes/zstd.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* Minimum supported version of ZSTD is now 1.4.0, for code simplification. Obsolete `CompressionType` `kZSTDNotFinalCompression` is also removed.
8 changes: 4 additions & 4 deletions util/compression.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ int ZSTDStreamingCompress::Compress(const char* input, size_t input_size,
if (input_size == 0) {
return 0;
}
#ifndef ZSTD_ADVANCED
#ifndef ZSTD
(void)input;
(void)input_size;
(void)output;
Expand Down Expand Up @@ -77,7 +77,7 @@ int ZSTDStreamingCompress::Compress(const char* input, size_t input_size,
}

void ZSTDStreamingCompress::Reset() {
#ifdef ZSTD_ADVANCED
#ifdef ZSTD
ZSTD_CCtx_reset(cctx_, ZSTD_ResetDirective::ZSTD_reset_session_only);
input_buffer_ = {/*src=*/nullptr, /*size=*/0, /*pos=*/0};
#endif
Expand All @@ -91,7 +91,7 @@ int ZSTDStreamingUncompress::Uncompress(const char* input, size_t input_size,
if (input_size == 0) {
return 0;
}
#ifdef ZSTD_ADVANCED
#ifdef ZSTD
if (input) {
// New input
input_buffer_ = {input, input_size, /*pos=*/0};
Expand All @@ -113,7 +113,7 @@ int ZSTDStreamingUncompress::Uncompress(const char* input, size_t input_size,
}

void ZSTDStreamingUncompress::Reset() {
#ifdef ZSTD_ADVANCED
#ifdef ZSTD
ZSTD_DCtx_reset(dctx_, ZSTD_ResetDirective::ZSTD_reset_session_only);
input_buffer_ = {/*src=*/nullptr, /*size=*/0, /*pos=*/0};
#endif
Expand Down
Loading

0 comments on commit 991a239

Please sign in to comment.