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

optimize: Use klauspost/compress for gzip #5

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

Conversation

klauspost
Copy link

What type of PR is this?

Optimization

What this PR does / why we need it (English/Chinese):

Switches to faster gzip compression.

See cloudwego/hertz#343 for more details

Upgrades dependencies to latest.

Which issue(s) this PR fixes:

Fixes cloudwego/hertz#343

Fixes cloudwego/hertz#343

Upgrades dependencies to latest
@CLAassistant
Copy link

CLAassistant commented Apr 14, 2023

CLA assistant check
All committers have signed the CLA.

@li-jin-gou
Copy link
Collaborator

refer to https://github.com/hertz-contrib/gzip/tree/optimize/gzip and the benefits are currently being verified on a service with a large Gzip CPU share.

@welkeyever
Copy link

It's better to keep the former one - try not to break the compatibility of existing users.

@klauspost
Copy link
Author

Feel free to do what suits you.

With regards to the PR, the only thing I noticed is that you use level 6 as default. I'd recommend to set it to 5. The difference isn't big in terms of compression - we are getting close to diminishing returns. And the 10-20% less CPU will probably be better spent on something else.

https://user-images.githubusercontent.com/5663952/199524248-54720258-c114-4424-87fa-ffb587de5120.png

That said it is only a very small difference. Let me know of your test results, or if there is anything else I can assist you with.

Regarding Gunzip, when the Reader is one of the possible stdlib provided ones it will be faster at decompression.

@li-jin-gou
Copy link
Collaborator

li-jin-gou commented Apr 17, 2023

Feel free to do what suits you.

With regards to the PR, the only thing I noticed is that you use level 6 as default. I'd recommend to set it to 5. The difference isn't big in terms of compression - we are getting close to diminishing returns. And the 10-20% less CPU will probably be better spent on something else.

https://user-images.githubusercontent.com/5663952/199524248-54720258-c114-4424-87fa-ffb587de5120.png

That said it is only a very small difference. Let me know of your test results, or if there is anything else I can assist you with.

Regarding Gunzip, when the Reader is one of the possible stdlib provided ones it will be faster at decompression.

Thanks 💕 and I will push the service to cut to klauspost/compress as soon as possible to get the CPU and PCT99 gains and let you know. cc @klauspost

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.

Use better gzip/deflate
4 participants