Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ntuple] Simplify and clean up compression #17847

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Feb 28, 2025

... eventually removing all member methods of RNTuple(De)Compressor; see the individual commits for more details. As a visible result, this saves 16 MiB of internal compression buffer per page sink.

@hahnjo hahnjo self-assigned this Feb 28, 2025
@hahnjo hahnjo requested a review from jblomer as a code owner February 28, 2025 12:26
Copy link
Contributor

@silverweed silverweed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thanks!

Copy link

github-actions bot commented Feb 28, 2025

Test Results

    18 files      18 suites   4d 8h 13m 37s ⏱️
 2 719 tests  2 715 ✅ 0 💤  4 ❌
47 253 runs  47 233 ✅ 0 💤 20 ❌

For more details on these failures, see this check.

Results for commit 5423345.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@enirolf enirolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! I have one small doc-related comment but otherwise it's good to go.

hahnjo added 6 commits March 3, 2025 14:23
InitImpl, CommitClusterGroupImpl, and CommitDatasetImpl already
allocate a buffer that is large enough and it is not needed to pass
via the internal buffer of RNTupleCompressor just to memcpy the
compressed contents out again.
It was already unused in SealPage since commit 208061d ("fix
unbuffered compression of large pages"). This saves 16 MiB of
internal compression buffer per page sink.
A RNTupleDecompressor will internally allocate a buffer of 16 MiB,
which is not needed for the anchor. It was also the only usage of
in-place decompression with RNTupleDecompressor.
They are not used anymore in the source code.
They are not used anymore in the source code.
@hahnjo hahnjo force-pushed the ntuple-compressor branch from 5423345 to d1b534d Compare March 3, 2025 13:26
@hahnjo
Copy link
Member Author

hahnjo commented Mar 3, 2025

(just rebasing to get a green CI before merging)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants