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

Fix compression ratio inefficiency #1709

Merged
merged 12 commits into from
Aug 5, 2019
Merged

Fix compression ratio inefficiency #1709

merged 12 commits into from
Aug 5, 2019

Conversation

Cyan4973
Copy link
Contributor

@Cyan4973 Cyan4973 commented Aug 1, 2019

Fix compression ratio inefficiency
introduced in #1624
under following scenario :

  • dictionary compression
  • at levels 1 to 3
  • with medium size payloads (larger than attachDict threshold)

The patch restore compression ratio to previous level.

A new compression efficiency test is added into fuzzer/unitTests in order to control gross negative variations in compression ratio, playing all levels with normal compression and flat dictionary compression.

Cyan4973 added 2 commits August 1, 2019 15:58
@Cyan4973
fixed compression ratio regression when dictionary-compressing medium…
98692c2 
…-size inputs at levels 1-3
@Cyan4973
added efficiency test
810a9ca 
to detect gross CR variations after a patch.

Tests normal and dictionary compression.
@Cyan4973
minor : fixed ptr arithmetic
2115292 
invalid on void ptr
@terrelln
Copy link
Contributor

terrelln commented Aug 1, 2019

Do you want to fix the rest of the strategies in this diff as well? The repro that exposes the bad behavior for every level is:

> head -c 40000 /dev/urandom > rand
> cp rand dict
> head -c 40000 /dev/urandom >> dict
> ./zstd -$LEVEL rand -D dict -o /dev/null
rand                 :100.04%   ( 40000 =>  40014 bytes, /dev/null)

@Cyan4973
Copy link
Contributor Author

Cyan4973 commented Aug 1, 2019

Yes, I will

Cyan4973 added 4 commits August 2, 2019 14:21
@Cyan4973
fixed strategies greedy, lazy & lazy2
5cf1b24 
restore dictionary compression ratio
@Cyan4973
fixed strategy btlazy2
b4257b0
@Cyan4973
fixed strategies btopt+
98e7c34
@Cyan4973
regenerate sample to compress
d1927f0 
to reduce chances of differences between 32 and 64-bit fuzzer tests
@Cyan4973
Copy link
Contributor Author

Cyan4973 commented Aug 2, 2019 •
edited
Loading

All strategies fixed, able to access the entire dictionary as long as at least one byte is within window size.

Still need to understand why the new size-control test fails on gcc-6 + 32-bit.

Also worthwhile to note :
the usan 32-bit build complains about integer overflow issues when reaching overflow protection tests for large windowLog.
It's a completely separate issue, but might be worth looking into (as part of another PR).

Cyan4973 added 2 commits August 2, 2019 17:34
@Cyan4973
fixed datagen
37f47e5 
to produce same content on both 32 and 64-bit platforms
by removing floating from literal table determination.

also : added checksum trace in compression control test,
so that it's easier to determine if test fails
as a consequence of compressing a different sample.
@Cyan4973
fixed minor conversion warning in datagen
387e20d
@Cyan4973
Copy link
Contributor Author

Cyan4973 commented Aug 2, 2019 •
edited
Loading

The failing test due to lack of reproducibility between 32 and 64-bit builds is now fixed.

And now, another completely unrelated test fails later in the serie.
As one can already guess, this test wasn't modified by a single line.

Great outcome, such a joy ....

@Cyan4973
minor test refactoring
efe8496 
just for clarity, for the currently failing unit test
@terrelln
Copy link
Contributor

terrelln commented Aug 2, 2019

As one can already guess, this test wasn't modified by a single line.

I'd love to switch these tests to gtest, given the time.

terrelln
terrelln approved these changes Aug 2, 2019
View reviewed changes
Copy link
Contributor

@terrelln terrelln left a comment

Choose a reason for hiding this comment

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

Just one comment, but I'll go ahead and accept so you can merge when its fixed.

lib/compress/zstd_lazy.c Outdated
U32 const lowestValid = ms->window.lowLimit;
U32 const withinWindow = (current - lowestValid > maxDistance) ? current - maxDistance : lowestValid;
U32 const isDictionary = (ms->loadedDictEnd != 0);
U32 const windowLow = isDictionary ? lowestValid : withinWindow;
Copy link
Contributor

Choose a reason for hiding this comment

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

The only variable you need for the rest of the function is windowLow right?

Can you extract this out into a function since it is duplicated in every match finder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's a good point

Cyan4973 added 2 commits August 3, 2019 16:51
@Cyan4973
fix test 122
0b0b83e 
it's an unsupported scenario.
@Cyan4973
factored the logic selecting lowest match index
facbe8b 
as suggested by @terrelln
@Cyan4973
Copy link
Contributor Author

Cyan4973 commented Aug 5, 2019

Last item completed, with windowLow logic factored into zstd_compress_internal.h .

@Cyan4973 Cyan4973 merged commit b2e71fa into dev Aug 5, 2019
@felixhandte felixhandte mentioned this pull request Aug 19, 2019
Merged
@Cyan4973 Cyan4973 deleted the fix1624 branch August 27, 2019 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terrelln terrelln terrelln 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.
3 participants
@Cyan4973 @terrelln @facebook-github-bot