Put `windowFrac` in the CCtxParams, not the CParams #4305

felixhandte · 2025-02-20T18:24:59Z

This PR is a set of commits stacked on top of #4275.

It refactors much of that PR, by moving where we store the windowFrac parameter from the CParams to instead storing it in the CCtxParams. This resolve the unpleasant part of that PR, which required splitting the CParams (and Params) structs into public and private variants.

This version has its own downsides, which requires that we convert many of the places that we handle and pass CParams around to instead pass around CCtxParams. That includes switching the MatchState to point to a CCtxParams rather than hold a CParams. This also requires that the CDict hold a CCtxParams that its MatchState can point to.

A particularly annoying result of this refactor is that where previously there was often a straightforward path of resolution from compression level -> CParams -> CCtxParams, the CCtxParams now have to be used in two different ways. We have to use the CCtxParams where previously we would transport CParams. But in those places, the CCtxParams are incomplete and are different from the real CCtxParams (because only the CParams part is set), and have to be translated/resolved into the real params before they are actually used. But because they're the same type, this distinction is not easy to discover, and it's not clear which kind of CCtxParams a particular instance is at any given point.

One nice corollary though is that this change does allow us in some internal APIs to remove other parameters that are now redundant now that the function has access to the whole CCtxParams.

This PR is a pure refactor and intends to introduce no functional changes.

I continue to believe we should abandon this PR and merge #4275 as-is.

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.

For inputs smaller than `1 << ZSTD_WINDOWLOG_MIN` bytes, the pre-existing code in `ZSTD_adjustCParams_internal()` temporarily calculated an illegally small window log for the purposes of adjusting the hash- and chain-table sizes, before fixing that bad window log to comply with the minimums. The new code clamped the window log earlier and therefore didn't shrink the tables as much. This commit fixes that to mirror the pre-existing behavior. This resolves the remaining regression test differences.

This also means the CDict needs to store and init a CCtxParams object.

… on CCtxParams

…rates on CCtxParams Sometimes this is used to validate user-provided CParams, before they're allowed to affect the CCtxParams. Leave those cases as-is. Otherwise, convert uses of this function to pass in the whole CCtxParams so that, in the next commit, when we pull out the `windowFrac` from the CParams, we can still cross-validate it with the `windowLog` CParam.

Unwind the public/private distinction.

…olution Components

Cyan4973 · 2025-02-24T21:20:26Z