You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
perf(cpp): reuse the LZ4 decompression buffer across pages - #1089
LZ4Compressor::uncompress() allocated a fresh output buffer for every page and released it in after_uncompress(). It also guessed the decompressed size with a 1.5x / 2.5x / 3.5x / 4.5x / 255x retry ladder, re-allocating whenever a guess was too small — on every page.
Separately, LZ4Compressor had an empty destructor even though CompressorFactory placement-news it, so the buffers it held were never released on destruction.
Change
Keep one decompression buffer on the compressor, grow it only when a page needs more room, and release it in destroy() (the destructor now calls destroy()).
after_uncompress() no longer frees the buffer; it stays owned by the compressor for reuse.
Benchmark
The effect is within noise on the existing benchmarks (~1–3% of read time); the change is mainly allocation hygiene: after the first page a decompression does not allocate at all.
Testing
Full C++ suite: 1454 tests passed, including LZ4Test.AfterUncompressFreesParamNotMember.
LZ4Compressor::uncompress() allocated a fresh buffer for every page and
released it in after_uncompress(), and it guessed the output size with a
1.5x/2.5x/... retry ladder, re-allocating until the guess fit.
Keep one buffer on the compressor, grow it only when a page needs more
room, and release it in destroy() (the destructor now runs destroy() so
callers that free the placement-new'ed object do not leak it). Pages
after the first therefore decompress without any allocation.
Note the destructor change: LZ4Compressor previously had an empty
destructor although the object is placement-new'ed by CompressorFactory,
so buffers held here would otherwise never be freed.
Uncompress pages of growing then shrinking sizes through one compressor
and verify every page's contents, so a reused (or grown) buffer cannot
silently corrupt later pages. Also pin the contract that
after_uncompress() leaves the compressor usable for the next page.
❌ Patch coverage is 82.75862% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.55%. Comparing base (90c7862) to head (30730d6). ⚠️ Report is 2 commits behind head on develop.
Compressors are placement-new'd into mem_alloc() storage, so free() only
returned the storage and never ran ~Compressor(). That was harmless while
every compressor released its buffers in after_uncompress(), but LZ4 now
retains its decompression buffer for reuse, so every recovered single-page
chunk leaked one page (LeakSanitizer: RestorableTsFileIOWriterTest.*
failed on the Ubuntu ASan job).
Make free() call the (virtual) destructor before mem_free and drop the
explicit ~Compressor() calls in the chunk readers so nothing is destroyed
twice. Also guard the LZ4 ratio ladder against non-positive ratios and
make LZ4Compressor non-copyable, since it owns the reused buffer.
Tests: assert the buffer pointer is reused for a smaller page and that
factory-allocated compressors release the retained buffer (LeakSanitizer
catches the old behaviour).
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR improves LZ4 decompression allocation behavior by retaining and reusing a single decompression buffer across pages, and attempts to ensure retained buffers are released when compressors are freed.
Changes:
Add persistent uncompressed_buf_ capacity tracking and reuse it across uncompress() calls.
Stop freeing the decompression buffer in after_uncompress(); free it in destroy() / destructor instead.
Update CompressorFactory::free() and remove manual destructor calls from readers; add tests for buffer reuse and factory-free cleanup.
File
Description
cpp/test/compress/lz4_compressor_test.cc
Adds tests for buffer reuse across page sizes and for factory free releasing retained decompression buffers.
cpp/src/reader/chunk_reader.cc
Removes manual destructor call before CompressorFactory::free().
cpp/src/reader/aligned_chunk_reader.cc
Removes several manual destructor calls before CompressorFactory::free() for compressors in pools/columns.
Compute compressed_buf_len * ratio in double and reject values above
INT_MAX before the int conversion; the largest retry ratio is 255x and
the cast was undefined for huge inputs. Also drop the pointer-identity
assertion for a grown buffer (the allocator may reuse the address) and
fix a comment typo.
Parallel CTest processes can seed the name generator in the same clock
tick; one then removes the file another is opening (E_ALREADY_EXIST at
the fixture's open, observed as SEGFAULT / test failure on CI). Append
pid and a per-process counter, as the writer tests already do.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
LZ4 decompression uses the retry estimate instead of the retained buffer’s full capacity, causing avoidable failed attempts and unnecessary buffer growth when compression ratios change between pages.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Use the full retained buffer capacity for LZ4 decompression
cpp/src/compress/lz4_compressor.cc:146
LZ4 receives the current retry estimate rather than the retained buffer's full capacity. A page that already fits can therefore trigger repeated failed decompression attempts and unnecessary buffer growth when compression ratios change between pages. Pass uncompressed_buf_capacity_ as the destination capacity so decoding uses the available space before retrying.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
LZ4Compressor::uncompress()allocated a fresh output buffer for every page and released it inafter_uncompress(). It also guessed the decompressed size with a 1.5x / 2.5x / 3.5x / 4.5x / 255x retry ladder, re-allocating whenever a guess was too small — on every page.Separately,
LZ4Compressorhad an empty destructor even thoughCompressorFactoryplacement-news it, so the buffers it held were never released on destruction.Change
destroy()(the destructor now callsdestroy()).after_uncompress()no longer frees the buffer; it stays owned by the compressor for reuse.Benchmark
The effect is within noise on the existing benchmarks (~1–3% of read time); the change is mainly allocation hygiene: after the first page a decompression does not allocate at all.
Testing
Full C++ suite: 1454 tests passed, including
LZ4Test.AfterUncompressFreesParamNotMember.