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 dedicated dict search isSupported() requirements. #2540

Merged
merged 1 commit into from
Mar 18, 2021

Conversation

senhuang42
Copy link
Contributor

@senhuang42 senhuang42 commented Mar 17, 2021

Currently, asserts in zstd_lazy.c will fail if you try to compress with chainLog > 24 or hLog < cLog with dedicated dict search.

The following command fails on dev:
make -j zstd DEBUGLEVEL=1 && ./zstd [file] -D [dictFile] --zstd=clog=25,hlog=23 -7f

This PR adds the same checks that are assert()ed to be true in zstd_lazy.c when using dedicated dict search, and includes a unit test that fails without this fix.

Though, I will note that dedicated dict search at least seems to work fine when clog > 24 or hLog < cLog, so an alternative could just be to remove the asserts.

Copy link
Contributor

@felixhandte felixhandte left a comment

Choose a reason for hiding this comment

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

Cool. Looks good.

As I recall, these asserts are present for the following reasons:

assert(chainLog <= 24);

In the DDSS, the hashtable layout for each entry is:

  • 4 bytes: first match index
  • 4 bytes: second match index
  • 4 bytes: third match index
  • 1 byte: number of additional matches in the chain table
  • 3 bytes: index of head of chain in chain table

So we only have 24 bits to index into the chain table.

assert(hashLog >= chainLog)

We need this for DDSS table construction, because we first build the chain table in the hash table, and then sort the chains into the chain table proper.

@senhuang42 senhuang42 merged commit eace4ab into facebook:dev Mar 18, 2021
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