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.

Sign up for GitHub

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

Jump to bottom

Cap hashLog & chainLog to ensure that we only use 32 bits of hash #3438

Merged
merged 1 commit into from
Jan 20, 2023
Merged

Cap hashLog & chainLog to ensure that we only use 32 bits of hash #3438

merged 1 commit into from
Jan 20, 2023

Conversation

terrelln
Copy link
Contributor

  • Cap shortCache chainLog to 24
  • Cap row match finder hashLog so that rowLog <= 24
  • Add unit tests to expose all cases. The row match finder unit tests are only run in 64-bit mode, because they allocate ~1GB.

Fixes #3336

@terrelln terrelln requested a review from embg January 20, 2023 00:19
@terrelln terrelln force-pushed the 2023-01-19-cap-hash-chain-log branch from 77769e7 to f29bbd1 Compare January 20, 2023 00:20
Cyan4973
Cyan4973 reviewed Jan 20, 2023
View reviewed changes
tests/fuzzer.c

DISPLAYLEVEL(3, "test%3i : ZSTD_lazy attach dictionary with hashLog = 29 and searchLog = 4 : ", testNb++);
if (MEM_64bits()) {
ZSTD_CCtx_params* cctxParams = ZSTD_createCCtxParams();
Copy link
Contributor

@Cyan4973 Cyan4973 Jan 20, 2023 •
edited

Choose a reason for hiding this comment

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

cctxParams probably needs to be free at the end of each block.

embg
embg approved these changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@embg embg left a comment

Choose a reason for hiding this comment

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

Looks good, just asked a couple questions mostly for my own understanding.

lib/compress/zstd_compress.c Show resolved Hide resolved
tests/fuzzer.c Show resolved Hide resolved
@terrelln terrelln force-pushed the 2023-01-19-cap-hash-chain-log branch from f29bbd1 to 18b10d7 Compare January 20, 2023 19:00
@terrelln
Cap hashLog & chainLog to ensure that we only use 32 bits of hash
18b10d7 
* Cap shortCache chainLog to 24
* Cap row match finder hashLog so that rowLog <= 24
* Add unit tests to expose all cases. The row match finder unit tests
  are only run in 64-bit mode, because they allocate ~1GB.

Fixes facebook#3336
@terrelln terrelln merged commit 666944f into facebook:dev Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cyan4973 Cyan4973 Cyan4973 approved these changes

@embg embg embg approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cap hashlog for row based matchfinder, chainlog for short cache matchfinders
4 participants
@terrelln @Cyan4973 @embg @facebook-github-bot