Skip to content

perf(compression): initialize gzip codec state lazily - #47

Merged
lorenzhs merged 1 commit into
release-24.0.0from
lorenz/gzip-lazy-init
Sep 11, 2026
Merged

lorenzhs merged 1 commit into
release-24.0.0from
lorenz/gzip-lazy-init

Conversation

@lorenzhs

@lorenzhs lorenzhs commented Sep 11, 2026 •

Copy link
Copy Markdown
Collaborator

Rationale for this change

GZipCodec::Init() initializes compressor state and then immediately initializes decompressor state, which tears the compressor state down again. Decompression-only callers therefore pay for an unrelated zlib workspace allocation and initialization.

What changes are included in this PR?

GZipCodec::Init() now validates the compression level and window size without allocating codec state. The existing lazy paths in Compress(), Decompress(), MaxCompressedLen(), MakeCompressor(), and MakeDecompressor() initialize the state they actually use.

The gzip option tests cover compression level and window-size validation independently, including compression level zero.

Are these changes tested?

The Arrow library builds successfully with the enabled compression backends. Existing codec tests exercise one-shot compression, one-shot decompression, and streaming construction in CI, with focused option coverage for the validation retained by Init().

Are there any user-facing changes?

No API or format behavior changes. Gzip codec state is initialized on first use instead of during codec construction.

Creating a GZipCodec eagerly initialized compressor state and then immediately discarded it while initializing decompressor state. Callers using the streaming factories therefore paid for unrelated codec state during construction.

Keep construction-time option validation, but leave compressor and decompressor allocation to the existing lazy initialization paths. Extend the option tests to cover compression-level and window-size validation independently.
@lorenzhs
lorenzhs marked this pull request as ready for review September 11, 2026 13:32

@tobias-fire tobias-fire left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stamping, but feels a bit strange to remove the InitCompressor() call from the Init() function completely. I would prefer a solution where we keep it, e.g. by only creating the decompression codec once per range reader, instead of once per page. Maybe that would be a good idea anyways?

@lorenzhs lorenzhs changed the title perf(compression): initialize gzip codec state lazily feat(compression): add streaming decompressor factory Sep 11, 2026
@lorenzhs
lorenzhs marked this pull request as draft September 11, 2026 14:07
@lorenzhs
lorenzhs marked this pull request as ready for review September 11, 2026 14:08
@lorenzhs
lorenzhs force-pushed the lorenz/gzip-lazy-init branch from e29a56f to 47f7f73 Compare September 11, 2026 15:00
@lorenzhs lorenzhs changed the title feat(compression): add streaming decompressor factory perf(compression): initialize gzip codec state lazily Sep 11, 2026
@lorenzhs

lorenzhs commented Sep 11, 2026 •

Copy link
Copy Markdown
Collaborator Author

It's totally safe, the flow is essentially

auto status = InitCompressor();
assert(status.ok());
InitDecompressor();

and InitDecompressor de-initialises the compressor that InitCompressor just created, so this fix only leaves us without a decompressor at the end -- but everything that needs it ensures it's there.

@lorenzhs
lorenzhs marked this pull request as draft September 11, 2026 15:04
@lorenzhs
lorenzhs marked this pull request as ready for review September 11, 2026 15:26
@lorenzhs
lorenzhs merged commit f1bf5b2 into release-24.0.0 Sep 11, 2026
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants