Content size written into frame after ZSTD_resetCStream despite contentSizeFlag not being set

[indygreg]

I believe I found another regression in 1.1.4 as detected by python-zstandard's test suite.

The equivalent C code is something like the following:

ZSTD_parameters params;
memset(&params, 0, sizeof(params));
cctx = ZSTD_createCCtx()
cstream = ZSTD_createCStream()
ZSTD_initCStream_advanced(cstream, NULL, 0, params, 42); /* Note non-0 size */

/* Do a compression operation with cstream */
/* The produced frame doesn't have the content size written because fparams.contentSizeFlag isn't set */

ZSTD_resetCStream(cstream, 42); /* Note non-0 size */

/* Do a 2nd compression operation with cstream */

/* Regression: the produced frame has content size == 42 written despite fparams not being set. */

[Cyan4973]

Thanks @indygreg ,
it's very clearly described, it will be straightforward to fix.

Edit : use case reproduced

[Cyan4973]

OK, I initially thought it was a simple fix,
but in fact, it touches the definition of what we want the API to do.

I forgot about it, but this behavior of ZSTD_resetCStream() is intentional.
It doesn't mean it cannot be changed : after all, there's a reason this prototype is still in "experimental" section. Should another behavior feel more "logical" or more desirable, this can still be done.
But we probably need to precisely define what we want first. And this includes taking care of "edge" cases.

Current behavior :

• ZSTD_resetCStream() always fill frame header with content size if value is provided as argument of the function. It effectively means that it acts as if contentSizeFlag is always set (whatever its initial value).
• For frame header to not contain content size, just provide 0 as pledgedSrcSize.

Reasoning : pledgedSrcSize sole objective is to fill the frame header.
Both options (content size present, or not present) are easily available, so one can switch from one choice to the other depending on circumstances.

minor con : there is no way to provide a content size of 0. 0 is always translated as unknown.

Potential new behavior :

• ZSTD_resetCStream() strictly enforces previously selected compression parameters. If frameContentFlag is disabled, content size should not be provided in header.

Here are a few questions resulting from this choice :

• If frameContentFlag is enabled, what should 0 represent ? "empty" content, or "unknown" content size" ?
  • If "empty" : how to represent an unknown content size when this scenario arise ?
    • Should the CStream be re-init just to make this case possible ?
  • If 0 means "unknown", how to represent "empty" ? Make this case impossible, basically imitating previous behavior ?
• If frameContentFlag is disabled, but user nonetheless provides a pledgedSrcSize > 0, how should the API react to it ?
  • Do not fill frame header, effectively canceling "pledgedSrcSize" without notification ?
  • Issue an error code ?

I'm not completely sure that the new behavior would feel better. It seems to contain more edge cases to handle and document.

One of the issues here is that we handle within a single method the "reset" action and "content size" field.
Another possibility would be to separate both concepts, so that they do not overlap.
But it's a more drastic API change, it would break current API.
It's related to the ongoing "advanced API" effort, which is unfinished. And I believe it would be better to wait for this new API rather than making multiple changes, even for an "experimental" API.

[indygreg]

Thanks for the write-up.

One subject you didn't mention is that of compression parameter adjustment. I /thought/ some instances of pledgedSrcSize were used to adjust compression parameters. So by passing a known content size into ZSTD_resetCStream() (and similar functions), I thought parameters would be adjusted for subsequent operations. Although I see from the comment in zstd.h that ZSTD_resetCStream() explicitly states that compression parameters will be reused. So I may have my wires crossed (I filed a previous issue regarding compression parameter reuse).

For python-zstandard, it isn't a big deal to check the frame settings for the operation and conditionally pass pledgedSrcSize > 0. That should preserve the old behavior without requiring any upstream API changes. When the "advanced API" manifests, we'll likely switch to it.

An immediate improvement that could be made though is for zstd.h to document the behavior of pledgedSrcSize. So I'll not close this issue in case you feel that is appropriate.

[Cyan4973]

Yes, I didn't mention it because I wanted to avoid complexity.

Indeed, you are completely right, an initial intention was to automatically adapt compression parameters depending on srcSize.

But the situation has become more complex recently.
The "dynamic adaptation" was designed to reduce "initialization time" and improve compression ratio by using a different set of bets.

Now, when 2 consecutive operations on the same CCtx use "similar" parameters (which is a bit difficult to summarise, but defined in code), it triggers a special "continuation" mode, which effectively skips initialisation phase.
This strategy tends to produce even better results for speed. So it's best to not change compression parameters.

But it's all not completely good, because some bets are better for large files than for smaller ones. And btw, some parameters can be changed while keeping intact the capability to trigger "continuation".

So there's still room for improvement. It works, but is likely missing some opportunities here and there, due to complexity.
I'm actually trying to make this a bit clearer in the code.
I also rely on having good scenarios to measure, and show the impact. And this part is still too limited for my liking.

An immediate improvement that could be made though is for zstd.h to document the behavior of pledgedSrcSize

Yes, that's an absolute minimum.
I've been constrained by the "one line" format of the comment.
Maybe that should be dropped to provide clearer and more accurate information.

[Cyan4973]

Updated comments for ZSTD_resetCStream() :
https://github.com/facebook/zstd/blob/dev/lib/zstd.h#L598

[Cyan4973]

There have been many tiny changes lately, mostly invisible, to better deal with this topic.
The main issue we face is that pledgedSrcSize==0 used to mean "unknown", but now its meaning is changed to "empty" under some circumstances.
Well defining and ensuring correct implementation of these circumstances is a necessity, and proved a bit difficult to follow, because the call flow wasn't prepared for that.

This is now completed, contentSizeFlag now applies consistently the following rules :

• All one-pass functions, such as ZSTD_compress() , systematically enable contentSizeFlag, and srcSize==0 means "empty".
• All streaming functions without additional control on pledgedSrcSize, such as ZSTD_compressBegin() or ZSTD_initCStream(), always disable contentSizeFlag.
• Functions offering control over pledgedSrcSize but no control over frameParameters, like ZSTD_resetCStream(), keep the current convention that pledgedSrcSize==0 means "unknown". Any other value enables contentSizeFlag. The provided size will be written in the header, and controlled on closing the frame.
• Functions offering control over both pledgedSrcSize and frameParameters, such as for example ZSTD_compressBegin_advanced(), or the newer ZSTD_compressBegin_usingCDict_advanced(), strictly follow instructions in frameParameters. Hence pledgedSrcSize will only be used if contentSizeFlag is enabled within frameParameters, and in this case, 0 will mean "empty".

While the rules can be explained and are now consistently applied, they feel nonetheless complex.

I briefly considered the introduction of a macro value to tell explicitly ZSTD_SRCSIZE_UNKNOWN, thus keeping 0 to mean "empty". However, I'm worried of the impact on existing user base, since the convention that pledgedSrcSize==0 means "unknown" is in place since a long time now, and changing that to mean "empty" in a way that the compiler cannot detect will likely trigger a lot of runtime issues.

Another outcome of these rules is that CDict never decides frameParameters : these parameters are either hard-coded in the relevant function, or explicitly provided in argument. As a consequence, the function ZSTD_createCDict_advanced() will be changed, to only accept compressionParameters as argument, as a way to emphasize that it does not set any frameParameter value.

Ultimately, I believe all these issues will be solved by the new "advanced API" in preparation. This new API has been designed with this issue in mind from the ground up. For the time being though, that's all we have ...

[Cyan4973]

We are in the final run towards next release.
The modifications presented above will be present.
Of course, this thread remains available to ask questions or provide feedback on the new feature.