From 96b78feec1f326fbfcfee9714a0dcaaa1a3683e1 Mon Sep 17 00:00:00 2001 From: Daniel Zheng Date: Tue, 15 Aug 2023 14:34:39 -0700 Subject: [PATCH 1/3] Fix zstd optimization api usage ZSTD_c_windowLog should be set as log base 2 of max block size. Changing hardcoded value to be determined by this variable. Test: m libsnapshot Change-Id: I09be447b7f1e95cb72a6a42eddb4035f61a43aad --- .../include/libsnapshot/cow_compress.h | 4 ++-- .../libsnapshot/libsnapshot_cow/cow_compress.cpp | 15 +++++++-------- fs_mgr/libsnapshot/libsnapshot_cow/test_v2.cpp | 2 +- fs_mgr/libsnapshot/libsnapshot_cow/writer_v2.cpp | 7 ++++--- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/fs_mgr/libsnapshot/include/libsnapshot/cow_compress.h b/fs_mgr/libsnapshot/include/libsnapshot/cow_compress.h index 8add041cd..97974c414 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/cow_compress.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/cow_compress.h @@ -32,9 +32,9 @@ class ICompressor { static std::unique_ptr Gz(uint32_t compression_level); static std::unique_ptr Brotli(uint32_t compression_level); static std::unique_ptr Lz4(uint32_t compression_level); - static std::unique_ptr Zstd(uint32_t compression_level); + static std::unique_ptr Zstd(uint32_t compression_level, const int32_t BLOCK_SZ); - static std::unique_ptr Create(CowCompression compression); + static std::unique_ptr Create(CowCompression compression, const int32_t BLOCK_SZ); uint32_t GetCompressionLevel() const { return compression_level_; } diff --git a/fs_mgr/libsnapshot/libsnapshot_cow/cow_compress.cpp b/fs_mgr/libsnapshot/libsnapshot_cow/cow_compress.cpp index 466b93c25..71ac59f20 100644 --- a/fs_mgr/libsnapshot/libsnapshot_cow/cow_compress.cpp +++ b/fs_mgr/libsnapshot/libsnapshot_cow/cow_compress.cpp @@ -55,7 +55,8 @@ std::optional CompressionAlgorithmFromString(std::strin } } -std::unique_ptr ICompressor::Create(CowCompression compression) { +std::unique_ptr ICompressor::Create(CowCompression compression, + const int32_t BLOCK_SZ) { switch (compression.algorithm) { case kCowCompressLz4: return ICompressor::Lz4(compression.compression_level); @@ -64,7 +65,7 @@ std::unique_ptr ICompressor::Create(CowCompression compression) { case kCowCompressGz: return ICompressor::Gz(compression.compression_level); case kCowCompressZstd: - return ICompressor::Zstd(compression.compression_level); + return ICompressor::Zstd(compression.compression_level, BLOCK_SZ); case kCowCompressNone: return nullptr; } @@ -175,12 +176,10 @@ class BrotliCompressor final : public ICompressor { class ZstdCompressor final : public ICompressor { public: - ZstdCompressor(uint32_t compression_level) + ZstdCompressor(uint32_t compression_level, const uint32_t MAX_BLOCK_SIZE) : ICompressor(compression_level), zstd_context_(ZSTD_createCCtx(), ZSTD_freeCCtx) { ZSTD_CCtx_setParameter(zstd_context_.get(), ZSTD_c_compressionLevel, compression_level); - // FIXME: hardcoding a value of 12 here for 4k blocks, should change to be either set by - // user, or optimized depending on block size - ZSTD_CCtx_setParameter(zstd_context_.get(), ZSTD_c_windowLog, 12); + ZSTD_CCtx_setParameter(zstd_context_.get(), ZSTD_c_windowLog, log2(MAX_BLOCK_SIZE)); }; std::basic_string Compress(const void* data, size_t length) const override { @@ -326,8 +325,8 @@ std::unique_ptr ICompressor::Lz4(uint32_t compression_level) { return std::make_unique(compression_level); } -std::unique_ptr ICompressor::Zstd(uint32_t compression_level) { - return std::make_unique(compression_level); +std::unique_ptr ICompressor::Zstd(uint32_t compression_level, const int32_t BLOCK_SZ) { + return std::make_unique(compression_level, BLOCK_SZ); } void CompressWorker::Finalize() { diff --git a/fs_mgr/libsnapshot/libsnapshot_cow/test_v2.cpp b/fs_mgr/libsnapshot/libsnapshot_cow/test_v2.cpp index 0ecad9d2c..e59bd927f 100644 --- a/fs_mgr/libsnapshot/libsnapshot_cow/test_v2.cpp +++ b/fs_mgr/libsnapshot/libsnapshot_cow/test_v2.cpp @@ -480,7 +480,7 @@ TEST_P(CompressionTest, HorribleStream) { std::string expected = "The quick brown fox jumps over the lazy dog."; expected.resize(4096, '\0'); - std::unique_ptr compressor = ICompressor::Create(compression); + std::unique_ptr compressor = ICompressor::Create(compression, 4096); auto result = compressor->Compress(expected.data(), expected.size()); ASSERT_FALSE(result.empty()); diff --git a/fs_mgr/libsnapshot/libsnapshot_cow/writer_v2.cpp b/fs_mgr/libsnapshot/libsnapshot_cow/writer_v2.cpp index 7115821b6..4172fc9c3 100644 --- a/fs_mgr/libsnapshot/libsnapshot_cow/writer_v2.cpp +++ b/fs_mgr/libsnapshot/libsnapshot_cow/writer_v2.cpp @@ -184,7 +184,8 @@ void CowWriterV2::InitWorkers() { return; } for (int i = 0; i < num_compress_threads_; i++) { - std::unique_ptr compressor = ICompressor::Create(compression_); + std::unique_ptr compressor = + ICompressor::Create(compression_, header_.block_size); auto wt = std::make_unique(std::move(compressor), header_.block_size); threads_.emplace_back(std::async(std::launch::async, &CompressWorker::RunThread, wt.get())); compress_threads_.push_back(std::move(wt)); @@ -341,7 +342,7 @@ bool CowWriterV2::CompressBlocks(size_t num_blocks, const void* data) { compressed_buf_.clear(); if (num_threads <= 1) { if (!compressor_) { - compressor_ = ICompressor::Create(compression_); + compressor_ = ICompressor::Create(compression_, header_.block_size); } return CompressWorker::CompressBlocks(compressor_.get(), options_.block_size, data, num_blocks, &compressed_buf_); @@ -416,7 +417,7 @@ bool CowWriterV2::EmitBlocks(uint64_t new_block_start, const void* data, size_t return data; } else { if (!compressor_) { - compressor_ = ICompressor::Create(compression_); + compressor_ = ICompressor::Create(compression_, header_.block_size); } auto data = compressor_->Compress(iter, header_.block_size); From 21aab6cb8384c3e7d53118c9684b450c7604b4b7 Mon Sep 17 00:00:00 2001 From: Daniel Zheng Date: Wed, 16 Aug 2023 15:19:57 -0700 Subject: [PATCH 2/3] Fix warning remove some unused headers and fix warning: Moving a temporary object prevents copy elision Test: m libsnapshot Change-Id: Idec3e051837dab5f1b95e677d1cdb09e9a57e73e --- fs_mgr/libsnapshot/libsnapshot_cow/cow_reader.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fs_mgr/libsnapshot/libsnapshot_cow/cow_reader.cpp b/fs_mgr/libsnapshot/libsnapshot_cow/cow_reader.cpp index f37aed1af..1b5d72484 100644 --- a/fs_mgr/libsnapshot/libsnapshot_cow/cow_reader.cpp +++ b/fs_mgr/libsnapshot/libsnapshot_cow/cow_reader.cpp @@ -17,9 +17,7 @@ #include #include -#include #include -#include #include #include #include @@ -103,7 +101,7 @@ bool CowReader::Parse(android::base::borrowed_fd fd, std::optional lab footer_ = parser.footer(); fd_size_ = parser.fd_size(); last_label_ = parser.last_label(); - ops_ = std::move(parser.ops()); + ops_ = parser.ops(); data_loc_ = parser.data_loc(); // If we're resuming a write, we're not ready to merge From 36882e98b4b33d48949cfcc3700f39f24489bebd Mon Sep 17 00:00:00 2001 From: Daniel Zheng Date: Tue, 15 Aug 2023 13:32:48 -0700 Subject: [PATCH 3/3] ZSTD read from wrong buf Fix zstd to read from ignore_buf rather than buf since that is where we are first copying the date Test: zstd ota Change-Id: I5032300e4628ecd7e49f1fa9f76dc9a828fb58e6 --- fs_mgr/libsnapshot/libsnapshot_cow/cow_decompress.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs_mgr/libsnapshot/libsnapshot_cow/cow_decompress.cpp b/fs_mgr/libsnapshot/libsnapshot_cow/cow_decompress.cpp index 3692c1a8a..2aaf3883f 100644 --- a/fs_mgr/libsnapshot/libsnapshot_cow/cow_decompress.cpp +++ b/fs_mgr/libsnapshot/libsnapshot_cow/cow_decompress.cpp @@ -351,7 +351,7 @@ class ZstdDecompressor final : public IDecompressor { return decompressed_size; } std::vector ignore_buf(decompressed_size); - if (!Decompress(buffer, decompressed_size)) { + if (!Decompress(ignore_buf.data(), decompressed_size)) { return -1; } memcpy(buffer, ignore_buf.data() + ignore_bytes, buffer_size);