Improve performance of Rabin fingerprint calculation #24
Conversation
This file contains 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
wpos is once again limited using modulo. The performance issue with > wpos = wpos % windowsSize Did not stem from the usage of modulo itself but from wpos being a signed integer which forces the compiler to emit a modulo implementation that can handle negative numbers instead of the much faster > wpos = wpos & 0x3f This optimized version is possible as windowSize is equal to 64. Performance: Old CPU: Intel(R) Xeon(R) CPU E5506 @ 2.13GHz benchmark old MB/s new MB/s speedup BenchmarkChunker 218.53 239.68 1.10x Modern CPU: Intel(R) Xeon(R) CPU E3-1275 v5 @ 3.60GHz benchmark old MB/s new MB/s speedup BenchmarkChunker 542.50 542.46 1.00x
Bounding wpos before using it or afterwards makes in general no difference. Here it allows the compiler to elide array bound checks when accessing win. Performance: Old CPU: Intel(R) Xeon(R) CPU E5506 @ 2.13GHz benchmark old MB/s new MB/s speedup BenchmarkChunker 239.68 243.10 1.01x Modern CPU: Intel(R) Xeon(R) CPU E3-1275 v5 @ 3.60GHz benchmark old MB/s new MB/s speedup BenchmarkChunker 542.46 547.28 1.01x
The code only uses polynomials of degree up to 53. Therefore let the compiler know of this restriction which avoids costly checks to handle the case where polShift is larger than the value type width, which would result in unexpected behavior on x86. > byte(digest >> polShift) Performance: Old CPU: Intel(R) Xeon(R) CPU E5506 @ 2.13GHz benchmark old MB/s new MB/s speedup BenchmarkChunker 243.10 268.83 1.11x Modern CPU: Intel(R) Xeon(R) CPU E3-1275 v5 @ 3.60GHz benchmark old MB/s new MB/s speedup BenchmarkChunker 547.28 602.37 1.10x
|
The speedups shown in the commit messages are still valid (to a large degree) for recent go versions such as 1.13.5 . |
|
Oh, wow, this PR fell through the cracks, I'm sorry for not responding earlier. I've rerun the benchmarks 10 times and benchstat tells me that it's a solid 8-10% speedup for my CPU (Intel(R) Core(TM) i5-6440HQ CPU @ 2.60GHz): Thank you very much! |
fd0
approved these changes
Feb 12, 2020
fd0
added a commit
that referenced
this pull request
Feb 12, 2020
Improve performance of Rabin fingerprint calculation
|
I've also released 0.3.0 with all the new commits. |
|
Should I open a pull request to update the chunker version in restic? |
|
Already done :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
These patches improve the chunking throughput by about 10-23% depending on the CPU used. The speedup is more noticeable on older CPUs.
The test were run on Linux using go 1.11.6 with
go test -bench 'BenchmarkChunker$' -cpu 1 -benchtime 20s. I've also repeated the complete performance measurements three times. As the results were quite stable (except for 1 extreme slow outlier), I've just included the results from the second measurement run.Would it be possible to create a new release for use in restic? The version there is already missing commit 7c2a180 which improves the performance to the baseline for the first commit in the merge request. For tag v0.2.0, which is the version used in restic, I've measured the following throughput:
This totals up to 15-36% more throughput than v0.2.0 .