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

ZSTD_CCtx_setCParams #3403

Merged
merged 2 commits into from
Dec 28, 2022
Merged

ZSTD_CCtx_setCParams #3403

merged 2 commits into from
Dec 28, 2022

Conversation

Cyan4973
Copy link
Contributor

Inspired by @terrelln in #3395,
offer a new capability to set all parameters defined in a ZSTD_compressionParameters structure with a single symbol invocation to improve user code brevity.

Inspired by #3395,
offer a new capability to set all parameters defined in a ZSTD_compressionParameters structure
with a single symbol invocation
to improve user code brevity.
Copy link
Contributor

@embg embg left a comment

Choose a reason for hiding this comment

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

LGTM, just one small nit and one potential issue that might require changes (hence, I am requesting changes). Feel free to land if changes aren't required.

lib/compress/zstd_compress.c Show resolved Hide resolved
lib/zstd.h Show resolved Hide resolved
@Cyan4973 Cyan4973 requested a review from embg December 28, 2022 21:18
@Cyan4973 Cyan4973 merged commit 481a2e1 into dev Dec 28, 2022
@embg
Copy link
Contributor

embg commented Dec 28, 2022

Hm... I just thought of a possible issue. You are assuming all the parameters in ZSTD_compressionParameters are authorized for mid-frame updates. However, isn't windowLog not authorized?

@Cyan4973
Copy link
Contributor Author

That's a very good point @embg .
windowLog can't change during compression.
And I actually assumed that windowlog's stickiness was taken care of somewhere later in the pipeline.

That's indeed the case :
https://github.com/facebook/zstd/blob/dev/lib/compress/zstdmt_compress.c#L1072

That being said, I agree this situation is not clear for users, so adding a code comment to specify this behavior would be helpful.

@embg
Copy link
Contributor

embg commented Dec 28, 2022

That's indeed the case : https://github.com/facebook/zstd/blob/dev/lib/compress/zstdmt_compress.c#L1072

Thanks for the pointer. Another question for my own understanding: how does this work for single-threaded compression? E.g. if ZSTD_CCtx_setCParams() is called in the middle of a frame, during streaming compression?

@Cyan4973
Copy link
Contributor Author

Another question for my own understanding: how does this work for single-threaded compression? E.g. if ZSTD_CCtx_setCParams() is called in the middle of a frame, during streaming compression?

If (allowed) parameters are changed during single-threaded streaming compression, actually nothing changes for the current compression session.

Changes are merely registered into requestedParameters, while ongoing compression uses appliedParameters.

@Cyan4973 Cyan4973 deleted the setCParams branch January 13, 2023 04:27
@Cyan4973 Cyan4973 mentioned this pull request Feb 9, 2023
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