Support Fractional-Power-of-Two Window Sizes in Compression #4275
Conversation
| @@ -22,110 +22,110 @@ | |||
| __attribute__((__unused__)) | |||
| #endif | |||
|
|
|||
| static const ZSTD_compressionParameters ZSTD_defaultCParameters[4][ZSTD_MAX_CLEVEL+1] = { | |||
| static const ZSTD_CParams ZSTD_defaultCParameters[4][ZSTD_MAX_CLEVEL+1] = { | |||
There was a problem hiding this comment.
Another way to pass this new parameter is to store it directly in the existing ZSTD_CCtx_params structure, which is internal only, can be updated anytime,
and is already used by many other advanced parameters,
such as for example the recently added ZSTD_c_blockSplitterLevel.
That's my recommendation to introduce any parameter that goes beyond ZSTD_compressionParameters definition.
There was a problem hiding this comment.
I'm not convinced by the strategy of creating a new intermediate internal structure to pass the new parameter.
We have an established pattern to add new parameters via ZSTD_CCtx_params.
I prefer we stick to it and don't introduce a new pattern specific to this new parameter.
|
I agree that it would have been more expedient to just put it in the In this future where the Zstd compression side supports fractional window sizes, it is a bug if you look at just the And, as we discussed, the match-finders need access to this parameter, so it needs to be available from the I considered an alternate approach where, rather than introduce a new variable, we change the internal |
Even though the `ZSTD_compressionParameters` struct is part of the unstable API, and therefore we are allowed to modify it, it seems likely that to actually do so would cause widespread chaos and bloodshed. So we are strongly incentivized to avoid changing it. We would, however, like to modify the CParams that are actually passed around internally. In particular, we want to be able to configure non-power-of-2 window sizes. So we need something more expressive than an integral window log. And we want this new field to be next to the window log, rather than hanging out in the CCtxParams, which are not passed to the block compressors. So, in order to support that, this commit: 1. Introduces a new struct, the `ZSTD_CParams`, that (for the moment) mirrors the definition of the public `ZSTD_compressionParameters` struct. 2. Codemods all internal use and storage of `cparams` internally to use the new struct definition. (The exception to this is the `ZSTD_parameters` struct, which is a public definition but is also passed around internally sometimes.) 3. Adds translation functions to convert the public and private struct defs to each other. 4. Uses those translation functions at the user API boundary (and when handling `ZSTD_parameters`).
This commit extends the refactor done in the previous and applies it to the `ZSTD_parameters` public struct, introducing an internal variant called `ZSTD_Params` whose only difference is containing a `ZSTD_CParams` instead of a `ZSTD_compressionParameters`. This commit similarly introduces conversion functions and rewrites internal functions to use the new, private definition. This allows us to clean up some internal conversions of the private `CParams` back into the public `CParams` in order to put them in a `ZSTD_parameters` struct to pass into something.
Store it in the CParams. Don't use it yet.
Using a blocksize of 100KB with a 65KB dictionary just *happens* to work with the current window-sizing logic, because it has to round all the way up to the next power of two. But it doesn't *have* to work. So this pre-emptively fixes up this test to use a size that will work after switching to tighter window sizing, but still uses a block size that is bigger than the dict size in case that matters for some reason?
We want to turn this on by default immediately after this release. And we want to thoroughly exercise the underlying code paths handling fractional windows, because they will be available in this upcoming release via new APIs (the explicit CCtx param and the constrain window for protocol path). And it's scary stuff. We can remove all these flags once we turn this on by default.
I found some slight errors via the exhaustive testing added to the fuzzer. This commit fixes them.
This PR adds support for configuring and performing compression with non-power-of-two window sizes.
The frame format and decoder have always supported more window sizes than just powers of two. The frame format defines a 3 bit window mantissa field in addition to the 4 bit window log descriptor. However, the compressor has only ever supported power-of-two windows. This PR changes that. In order to do that, it makes a number of changes:
windowFracfield to the private CParams (but not to the public one, because we are scared of changing that struct definition.ZSTD_c_windowFracwhich allows users to set this param.To-Do: