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

Bump Snappy to 1.1.10 #1309

Closed
2 tasks done
aleksraiden opened this issue Mar 9, 2023 · 13 comments
Closed
2 tasks done

Bump Snappy to 1.1.10 #1309

aleksraiden opened this issue Mar 9, 2023 · 13 comments
Labels
enhancement type enhancement

Comments

@aleksraiden
Copy link
Contributor

Search before asking

  • I had searched in the issues and found no similar issues.

Motivation

We can try to use a newest release of Google Snappy library (1.10.0+, https://github.com/google/snappy/releases/tag/1.1.10).

But at now googler's broke a Clang support, so I'am waiting to fix release.

I remember, that we had a some bug with snappy early (e.g. #788), maybe in this release they are fixed. Let's try to check.

Solution

No response

Are you willing to submit a PR?

  • I'm willing to submit a PR!
@aleksraiden aleksraiden added the enhancement type enhancement label Mar 9, 2023
@git-hulk
Copy link
Member

git-hulk commented Mar 9, 2023

Thanks @aleksraiden, can use redis-benchmark -p 6666 -t set -d 102400 -r 100000000 -n 10000000 to check if the bug still exists.

@aleksraiden
Copy link
Contributor Author

@git-hulk Lot of thanks, I check this into my private fork asap.

@aleksraiden
Copy link
Contributor Author

A bug with corrupted compression block have no reproduce in all my tests - Debian 11 х64 and special test unit with CentOS 7.9.2009 х64.

@git-hulk
Copy link
Member

Thanks for your efforts.

@aleksraiden
Copy link
Contributor Author

This would be a largest test platform for me - 3Tb volume dedicated for kvrocks )

@aleksraiden
Copy link
Contributor Author

@torwig create a patch for Snappy with correct fix current problem (~1 LoC!!!! just see - google/snappy#165) but we have no idea about how long a Googlers will be thinking about it for a fix. So, could we can try a quick fix like LuaJIT - fork Google/Snappy repository into KVRocks lab, apply our fix and use a new Snappy codec right now. And in a future, when a new release of Snappy are shipped, we switch to use them.

What you think about this? @git-hulk @tisonkun @torwig @PragmaTwice @ShooterIT

@git-hulk
Copy link
Member

@aleksraiden I prefer to wait for @torwig's PR to be merged because the old version works well now. To see if others folks have any thoughts on this.

@tisonkun
Copy link
Member

  1. CMake supports such simple patches during the compile stage.
  2. Generally, we don't maintain too many hacks upstream, you can apply the patch and run the revision in your prod, that's how OSS stratifies customized usage.

So I'm -1 on forking a new repo, but choose either 1 or 2 above.

@aleksraiden
Copy link
Contributor Author

Ok, let's wait, I pinged one of the committers in snappy repo.

@aleksraiden
Copy link
Contributor Author

In a Snappy repo bug are fixed now, just waiting a new release tag.

@git-hulk
Copy link
Member

Hi @aleksraiden, great thanks for your follow-up.

@aleksraiden
Copy link
Contributor Author

So, I add a new version of Snappy (based on this commit google/snappy@f725f67), with (in addition to 1.10.0):

  • Optimization of decompression in ARM platform
  • Add prefetch for compression
  • Fix -Wsign-compare bug

Because a Snappy have a something like 1-Year release cycle, and because this (1.10) release have a lot of performance improvements, my propose is use this version without waiting a next release of them.

@aleksraiden
Copy link
Contributor Author

Done, now closing, thanks to all!

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

No branches or pull requests

3 participants