Performance improvements - #110
Closed
paulwouters wants to merge 2 commits into
Closed
Conversation
paulwouters
commented
Nov 18, 2021
paulwouters
commented
- stream compressors: use Intel SSE4.2 CRC32C instruction when crc32c is available
- StreamDecompressor: faster decompression via a lot less memcpy
added 2 commits
November 17, 2021 13:23
The decompression loop does a lot of copying of memory. Performance tests indicate more time is spend in copying memory than in the actual snappy decompression algorithm. Use memoryviews and bytearrays to reduce the number of memcpys. This gives about 24% faster execution on my test setup. NOTE: the interface is changed a bit: the decompressor returns a memoryview() into a bytearray() instead of a bytes() object. This may break existing code.
…ib available https://github.com/ICRAR/crc32c This library uses the Intel SSE4.2 CRC32C instruction, which is a lot faster than using the crc33c function that comes with the snappy library. The result is about 100% faster decompression on my test setup.
Member
|
I am not able to review the changes here - anyone else around? Do you know is snappy in cramjam can benefit from similar changes? (cc @milesgranger ) |
Author
|
For us, the difference is that without these two patches, decompressing ~50mb takes 13sec, ~100mb takes 60sec (this is somewhat data-dependent). With the patches applied over 0.6.0, decompressing that same 100mb takes 1sec |
Member
|
Wow! Such a difference doesn't entirely seem plausible :| |
I don't know, suppose the improvements would have to be in the upstream snappy crate. However, I installed this branch from source and re-ran the benchmarks on my end w/ crc32c installed, and I get very similar results to existing benchmarks Could be I didn't try hard enough, or the optimizations aren't triggered for my machine, but my system seems to align with what would be qualified for them supposedly. 🤷♂️ Snappy raw: Snappy framed |
|
Actually, it (python-snappy) does quite a bit better in the framed format with the |
Author
|
These are the benchmarks we are seeing with and without the patch using snappy's own benchmarking tools: without patch: With patch: That is a significant difference. We would really prefer not to have to maintain a separate patch indefinitely. But the performance increase is essential for our use. |
Member
|
Have you had a chance to examine the CRC error showing up in the tests? |
Author
|
crc errors? I am not sure what this is referring to? |
Member
|
See lines following https://github.com/andrix/python-snappy/runs/4255192362?check_suite_focus=true#step:7:312 (and also for one more of the test runs) |
Author
Those crc errors were due to python2 tests being run. A separate PR was filed to make it work with python2: #111 although there I got feedback python2 is not supported. So I'm stuck in a catch22 here. |
Member
|
Since this has sat a long time, and #111 failed to solve the py2 issue, let's drop py2 right now, right here, so that we can get this improvement in. |
Author
|
Did you close this by mistake ? This PR was fine but it caused failures in testing related to your pyton2 test cases that were still run despite not being officially supported. I see you closed the fix for that issue, which is fine. But I don't understand why this one was closed? |
Member
|
In the latest version of this package (now in dev release on pypi), we no longer use the C library of snappy at all, but defer to cramjam's version. As @milesgranger commented above, it is plausible that something similar could help there, but you would certainly need to rework some things. I think - happy to be proved wrong if these changes are still useful. |
|
I think these are the relevant changes in the Rust crate cramjam uses for snappy: BurntSushi/rust-snappy@204215c |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.