Conversation
WalkthroughThis pull request introduces several modifications to the benchmarking suite for compression and uncompression functions. The changes include adding a new benchmark file for uncompression performance testing, initializing variables in existing benchmark files to improve consistency, and updating the CMakeLists.txt to include the new benchmark source file. The modifications aim to enhance the testing infrastructure for compression-related functions by providing more comprehensive performance measurement capabilities. Changes
Sequence DiagramsequenceDiagram
participant Benchmark as Benchmark Framework
participant UncompressBench as Uncompress Benchmark
participant Uncompress as Uncompress Function
Benchmark->>UncompressBench: SetUp
UncompressBench-->>Benchmark: Allocate Buffers
UncompressBench->>UncompressBench: Compress Test Data
Benchmark->>UncompressBench: Bench
UncompressBench->>Uncompress: Call uncompress()
Uncompress-->>UncompressBench: Return Decompressed Data
Benchmark->>UncompressBench: TearDown
UncompressBench-->>Benchmark: Free Allocated Buffers
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/benchmarks/benchmark_uncompress.cc (2)
20-21: Consider making MAX_SIZE and NUM_TESTS configurable.These constants could be made configurable through CMake variables to allow for different test configurations without code changes.
67-70: Consider using a map for O(1) size lookup.The linear search in the benchmark loop could be replaced with a map for better performance:
private: + std::unordered_map<int64_t, size_t> size_to_index; // ... other members ... public: void SetUp(const ::benchmark::State& state) { // ... existing setup ... for (size_t i = 0; i < NUM_TESTS; ++i) { + size_to_index[sizes[i]] = i; // ... rest of the setup ... } } void Bench(benchmark::State& state) { int err = 0; for (auto _ : state) { - int index = 0; - while (sizes[index] != state.range(0)) ++index; + int index = size_to_index[state.range(0)]; // ... rest of the benchmark ... } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/benchmarks/CMakeLists.txt(1 hunks)test/benchmarks/benchmark_compare256_rle.cc(1 hunks)test/benchmarks/benchmark_compress.cc(1 hunks)test/benchmarks/benchmark_uncompress.cc(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: macOS GCC
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: macOS GCC
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: macOS GCC
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: macOS GCC
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: macOS GCC
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: macOS GCC
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: macOS GCC
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: macOS GCC
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: macOS GCC
- GitHub Check: EL9 Clang S390X DFLTCC MSAN
- GitHub Check: macOS GCC
🔇 Additional comments (5)
test/benchmarks/benchmark_compress.cc (1)
46-46: LGTM! Good practice to initialize the variable.Initializing
errto 0 before use prevents potential undefined behavior and silences compiler warnings.test/benchmarks/benchmark_compare256_rle.cc (1)
36-36: LGTM! Good practice to initialize the variable.Initializing
lento 0 before use prevents potential undefined behavior and silences compiler warnings.test/benchmarks/benchmark_uncompress.cc (2)
88-94: LGTM! Consistent with other benchmarks.The benchmark registration follows the established pattern and uses appropriate test sizes.
54-61: Consider validating compressed size.It would be good to verify that the compressed size is within expected bounds:
uLong compressed_size = maxlen; int err = PREFIX(compress)(compressed_buff[i], &compressed_size, inbuff, sizes[i]); if (err != Z_OK) { fprintf(stderr, "Compression failed with error %d\n", err); abort(); } + if (compressed_size >= maxlen) { + fprintf(stderr, "Compressed size %lu exceeds buffer size %lu\n", compressed_size, maxlen); + abort(); + } compressed_sizes[i] = compressed_size;test/benchmarks/CMakeLists.txt (1)
51-51: LGTM! Correctly added to benchmark target.The new benchmark file is properly added to the benchmark_zlib target and maintains alphabetical ordering.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1860 +/- ##
===========================================
+ Coverage 80.34% 80.36% +0.01%
===========================================
Files 138 139 +1
Lines 11147 11178 +31
Branches 2860 2868 +8
===========================================
+ Hits 8956 8983 +27
+ Misses 1223 1221 -2
- Partials 968 974 +6 ☔ View full report in Codecov by Sentry. |
Adds uncompress benchmark.
Also fix two minor warnings in other benchmarks.
Summary by CodeRabbit
Release Notes
New Features
uncompress()functionBug Fixes
Tests