Repository navigation
Conversation
ff2dfbc to
f97e8ab
Compare
Buffers larger than kMAXZIPBUF are compressed in several blocks. So far all blocks but the last were kMAXZIPBUF bytes long. For a buffer just above a multiple of kMAXZIPBUF, the last block is only a few bytes, which cannot be compressed, and the whole buffer was then stored uncompressed: RNTuple pages of 2^24 bytes (and every full page with a 32 MiB page size limit), TTree baskets, TKey objects and TMessages of that size. Cut the buffer into the same number of blocks, all of the same size instead (ROOT::Internal::GetZipBlockSize). Readers take each block's size from its header, so the format and the reading code are unchanged. Fixes root-project#23706
f97e8ab to
62cb787
Compare
Test Results 21 files 21 suites 3d 15h 12m 12s ⏱️ For more details on these failures, see this check. Results for commit 62cb787. |
| Int_t nbuffers = 1 + (fObjlen - 1)/kMAXZIPBUF; | ||
| const Int_t blockSize = ROOT::Internal::GetZipBlockSize(fObjlen); |
There was a problem hiding this comment.
What would happen if GetZipBlockSize::nblocks and nbuffers get out of sync? Should we either assert that (fObjlen + blockSize - 1) / blockSize == nbuffers or (probably better) pass nbuffers as a parameter? (or even return both nbuffers and blockSize for the function if we want to delegate the entire split to that function).
There was a problem hiding this comment.
I like the latter the most (returning both, block size and number of blocks from the helper function).
There was a problem hiding this comment.
I agree with this. I changed the helper in c13eb40: GetZipBlockSize to GetZipBlocks which implements this
jblomer
left a comment
There was a problem hiding this comment.
LGTM but see open comments
| /// stored uncompressed. | ||
| constexpr int GetZipBlockSize(long long nbytes) | ||
| { | ||
| const long long nblocks = 1 + (nbytes - 1) / kMAXZIPBUF; |
There was a problem hiding this comment.
| const long long nblocks = 1 + (nbytes - 1) / kMAXZIPBUF; | |
| assert(nbytes > 0); | |
| const long long nblocks = 1 + (nbytes - 1) / kMAXZIPBUF; |
| if (i == nbuffers - 1) bufmax = fObjlen - nzip; | ||
| else bufmax = kMAXZIPBUF; | ||
| else | ||
| bufmax = blockSize; |
There was a problem hiding this comment.
Maybe reformat the entire if-else block
| Int_t nbuffers = 1 + (fObjlen - 1)/kMAXZIPBUF; | ||
| const Int_t blockSize = ROOT::Internal::GetZipBlockSize(fObjlen); |
There was a problem hiding this comment.
I like the latter the most (returning both, block size and number of blocks from the helper function).
| /// smaller by less than the number of blocks. Full blocks followed by the remainder would end a buffer just above a | ||
| /// multiple of kMAXZIPBUF in a block of a few bytes, which cannot be compressed, so that the whole buffer would be | ||
| /// stored uncompressed. | ||
| constexpr int GetZipBlockSize(long long nbytes) |
There was a problem hiding this comment.
I would replace all instances of long long with std::uint64_t
There was a problem hiding this comment.
Done (c13eb40) for the parameter. The returned values stay int because the callers use Int_t and they're still smaller than kMAXZIPBUF
Replace GetZipBlockSize by GetZipBlocks, which returns the number of blocks and their size (RZipBlocks), so the five writers no longer compute the count themselves and both values cannot disagree. GetZipBlocks asserts a positive size. RNTupleCompressor::Zip and TBasket::WriteBuffer now handle empty buffers before calling it; the result for them is unchanged.
This Pull request:
Changes or fixes:
Buffers larger than
kMAXZIPBUF(2^24 − 1 bytes) are compressed in several blocks. So far all blocks but the last werekMAXZIPBUFbytes long, and if any block did not shrink the whole buffer was stored uncompressed. For a buffer just above a multiple ofkMAXZIPBUFthe last block is only a few bytes, which can never be compressed, so such buffers were always stored uncompressed (#23706): RNTuple pages of 2^24 bytes, every full page with a 32 MiB page size limit, and TTree baskets,TKeyobjects andTMessages of that size.This PR cuts the buffer into the same number of blocks as before, but all of (nearly) the same size:
ROOT::Internal::GetZipBlockSize(nbytes)inRZip.h, used by the five writers (RNTupleCompressor::Zip,TBasket::WriteBuffer, bothTKeyconstructors,TMessage::Compress). No block is larger thankMAXZIPBUF, and no block is tiny.The format does not change: all readers (
TBasket::ReadBasketBuffers,TKey::UnzipBuffer,TTreeCacheUnzip,TMessage::Uncompress,RNTupleDecompressor::Unzip) take each block's size from its header. Older ROOT versions read the new files. Buffers up tokMAXZIPBUFbytes are compressed exactly as before; larger ones now compress to different, valid bytes.Tests:
RNTupleZip.LargeWithOutputBufferasserted the old behaviour ("Trailing byte cannot be compressed, entire buffer returns uncompressed") and now checks that akMAXZIPBUF + 1buffer is compressed and read back. New:RNTupleZip.LargeJustAboveBlocks(sizeskMAXZIPBUF + 1,+ 2and 2^25, with ZLIB, LZMA, LZ4 and ZSTD) and, inio/io/test/ZipHeader.cxx,RZip.JustAboveBlockSize{TKey,TTree,Message}: an object, a basket and a message of exactlykMAXZIPBUF + 1bytes must be compressed and read back.Effect, with the reproducer of #23706 (ZSTD 5): RNTuple pages of 2^24, 2^24 + 1, + 19 and + 39 bytes take 566–574 bytes on storage instead of 16.8 MB; a TTree basket of 2^24 bytes takes 631 bytes instead of 16 777 281. On real data (CMS HGCAL hits written as RNTuple with 32 000 000-byte pages, 203 pages, 45 of them larger than one block), recompressing every page gives 312.9 instead of 346.5 MB with ZSTD 3, 308.3 instead of 341.8 MB with ZSTD 5 and 282.1 instead of 315.8 MB with LZMA 7; the difference is the pages that were stored uncompressed, the other pages hardly change.
On synthetic buffers (zeros, a compressible pattern and random data, 4 algorithms, sizes from
kMAXZIPBUF − 1to 10^8 bytes, 192 cases), 75 buffers that were stored uncompressed are now compressed, 94 are unchanged (all buffers up tokMAXZIPBUFand all random data), and 23 change by a few bytes or per cent in either direction. The largest increase is +5.5 % with LZMA, whose dictionary is capped at a quarter of each (now smaller) block.Checklist:
Tested on x86_64 (EL9, gcc 14.3) with two builds of 14a58f7 (master of 2026-10-12), with and without this change,
-Dminimal=ON -Dtesting=ON:ctest -R '^gtest-(io|tree|net)'passes (100 of 100) in both; the new tests fail without the change.This PR fixes #23706