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

Use max compression level in ZSTD #1111

Merged
merged 4 commits into from
Nov 22, 2019
Merged

Use max compression level in ZSTD #1111

merged 4 commits into from
Nov 22, 2019

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Nov 7, 2019

The default level is 5. This PR sets the compression level to 15 which gives the best speed vs compression ratio trade-off.

Level Compression Ratio Time Taken (Data Dog) (in n/s)
1 2.932754881 21152
2 2.753564155 21752
3 2.729475101 24112
4 2.816666667 38338
5 2.897142857 46792
6 2.954115076 69259
7 2.954115076 94859
8 2.954115076 122494
9 2.954115076 76904
10 2.954115076 75655
11 2.870488323 178359
12 3.09382151 418902
13 4.384864865 244562
14 4.384864865 242424
15 4.384864865 239042
16 4.38961039 471493
17 4.38961039 470330
18 4.38961039 472117
19 4.38961039 471703
20 4.38961039 474596

Compression Ratio and Time Taken (Data Dog) (in n_s)


This change is Reviewable

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Got a comment.

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ashish-goswami and @jarifibrahim)


y/zstd_cgo.go, line 35 at r1 (raw file):

// ZSTDCompress compresses a block using ZSTD algorithm.
func ZSTDCompress(dst, src []byte) ([]byte, error) {
	return zstd.CompressLevel(dst, src, 15)

Add a big comment here about the benchmark results that you did.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Let's allow the user to decide in our options. Get another PR. :lgtm_cancel:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ashish-goswami and @jarifibrahim)

@coveralls
Copy link

coveralls commented Nov 7, 2019

Coverage Status

Coverage decreased (-0.01%) to 77.344% when pulling 3285418 on ibrahim/zstd-level into cbbaefa on master.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 6 of 6 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ashish-goswami and @jarifibrahim)

@jarifibrahim jarifibrahim merged commit 3eb4e72 into master Nov 22, 2019
@jarifibrahim jarifibrahim deleted the ibrahim/zstd-level branch November 22, 2019 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants