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

zstd: encode+decode performance improvements #1869

Merged
merged 1 commit into from
Apr 29, 2021

Conversation

wyndhblb
Copy link
Contributor

@wyndhblb wyndhblb commented Jan 7, 2021

the lock in that once per decode/encode is expensive, so just do it once at init

In an empirical test, on a laptop of course, this gained about a few hundred msg/sec in both consumption/production

@wyndhblb wyndhblb requested a review from bai as a code owner January 7, 2021 16:24
@ghost ghost added the cla-needed label Jan 7, 2021
Copy link
Contributor

@d1egoaz d1egoaz left a comment

Choose a reason for hiding this comment

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

LGTM

I wonder if you could provide the results of any local benchmark, at least for document the improvements.

This will always initialize a new zstd enc/dec even if zstd is not used, but I don't think that will cause a memory issue in any app.

I was reading the other encoders initialization, and it looks for gzip and lz4 is using a sync.Pool https://github.com/Shopify/sarama/blob/65f0fec86aabe011db77ad641d31fddf14f3ca41/compress.go#L14

not sure TBH why that was needed and why snappy or zstd is not using that pattern.

@dnwe
Copy link
Collaborator

dnwe commented Mar 15, 2021

@wyndhblb please can you sign the CLA and let us know so we can re-run the status check?
https://cla.shopify.com/

@dnwe
Copy link
Collaborator

dnwe commented Mar 15, 2021

You may also need your commit username+e-mail to be registered against your GitHub account too, it looks like they're not currently?

@dnwe dnwe changed the title Performance improvement zstd: encode+decode performance improvements Mar 15, 2021
@bai
Copy link
Contributor

bai commented Mar 27, 2021

Hey @wyndhblb 👋🏼 Wanted to do a friendly follow-up and see if you could sign the CLA, so we could get this merged? 🙏🏼

@wyndhblb
Copy link
Contributor Author

@bai i have a long long time ago.

@d1egoaz
Copy link
Contributor

d1egoaz commented Apr 6, 2021

@wyndhblb we're getting this issue from the CLA check

image

the lock in that once per decode/encode is expensive, so just do it once
at init

Signed-off-by: bo <[email protected]>
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.

4 participants