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 windowLog comparison in Zstd boilerplate #58

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andywilco
Copy link

@andywilco andywilco commented Oct 23, 2019

Comparison sense is reversed so windowLog larger than default is never applied.

@inikep inikep force-pushed the master branch 3 times, most recently from a90547a to 440a55f Compare October 24, 2019 08:07
@inikep
Copy link
Owner

inikep commented Oct 24, 2019

The windowLog is used to limit an LZ dictionary window for e.g zstd22:

Compressor name         Compress. Decompress. Compr. size  Ratio
zstd 1.4.3 -19           3.73 MB/s  1078 MB/s    53254304  25.13
zstd22 1.4.3 -19         4.28 MB/s  1096 MB/s    54110538  25.53

@andywilco
Copy link
Author

Hmm maybe I need a more comprehensive fix. It seems reasonable to pass a larger windowLog much like the --long argument, otherwise windowLog is capped at default. zstd30LDM added for example.

Without patch:

Compressor name         Compress. Decompress. Compr. size  Ratio
zstd 1.3.8 -1             318 MB/s   680 MB/s    19468263  15.21
zstdLDM 1.3.8 -1          144 MB/s   712 MB/s    18639432  14.56
zstd22LDM 1.3.8 -1        143 MB/s   715 MB/s    18639432  14.56
zstd24LDM 1.3.8 -1        146 MB/s   721 MB/s    18639432  14.56
zstd30LDM 1.3.8 -1        147 MB/s   726 MB/s    18639432  14.56 

With patch:

Compressor name         Compress. Decompress. Compr. size  Ratio
zstd 1.3.8 -1 317 MB/s 711 MB/s 19468263 15.21
zstdLDM 1.3.8 -1 146 MB/s 730 MB/s 18639432 14.56
zstd22LDM 1.3.8 -1 138 MB/s 758 MB/s 17545488 13.71
zstd24LDM 1.3.8 -1 136 MB/s 833 MB/s 15620953 12.20
zstd30LDM 1.3.8 -1 124 MB/s 893 MB/s 13035351 10.18

@inikep
Copy link
Owner

inikep commented Oct 25, 2019

I think it can be done by renaming windowLog with limitWindowLog and adding a new parameter increaseWindowLog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants