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

Fix dictionary force reloading clevel selection #2570

Merged
merged 2 commits into from
Apr 6, 2021

Conversation

senhuang42
Copy link
Contributor

As title - reloading the dictionary with ZSTD_dictForceLoad was basically broken because we would always choose the default compression level since initLocalDict() always sets a cdict. Now, we only override cctx clevel with cdict's clevel if the cctx had the cdict to begin with.

results.csv updated accordingly.

@senhuang42 senhuang42 changed the title CDict clevel superseding should occur before initLocalDict() Fix dictionary force reloading clevel selection Apr 5, 2021
@senhuang42
Copy link
Contributor Author

senhuang42 commented Apr 6, 2021

Investigating the surprising non determinism failures... these are locally reproducible too. I wonder if this is exposing an existing bug or is the cause itself.

Edit: Seems to be because subsequent calls of compressStream2() that have a localDict would appear to have a dedicated cdict, and therefore we'd end up taking that cdict's level when we shouldn't. So, we should check that there is no localDict.cdict as well.

@senhuang42 senhuang42 merged commit e381245 into facebook:dev Apr 6, 2021
Comment on lines +5167 to +5168
if (cctx->cdict && !cctx->localDict.cdict)
params.compressionLevel = cctx->cdict->compressionLevel; /* let cdict take priority in terms of compression level */
Copy link
Contributor

@terrelln terrelln Apr 6, 2021

Choose a reason for hiding this comment

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

@senhuang42 I think this is correct, but it is really confusing to have moved this above ZSTD_initLocalDict(). When ZSTD_initLocalDict() is creates a cdict, this will never run, because necessarily cctx->cdict == nullptr in that case. Since ZSTD_CCtx_loadDictionary() first calls ZSTD_clearAllDicts().

I believe that leaving this line in the original location (L5170), with the additional !cctx->localDict.cdict check will behave identically, but is much clearer.

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.

4 participants