Simplify Repository.LoadBlob usage#2640
Merged
MichaelEischer merged 2 commits intorestic:masterfrom Apr 23, 2020
Merged
Conversation
0fe2a73 to
49e582d
Compare
Contributor
Author
|
I also tested this with restic dump on a 4GB file. It's neither slower, nor faster. The SHA256 checks out. |
Member
|
So I guess this obsoletes #2629 ? |
Contributor
Author
|
Yes. Sorry, I had forgotten about that one. |
MichaelEischer
approved these changes
Apr 19, 2020
Member
MichaelEischer
left a comment
There was a problem hiding this comment.
I've got just two minor points, see the review comments. Once these and the merge conflict are resolved, the PR is ready for merging.
7 tasks
251e80f to
096c495
Compare
Contributor
Author
|
I've merged with the earliest commit on master that resolved the merge conflict to get the CI working again. I hope that's ok. |
657c119 to
726cc87
Compare
Member
|
Could you rebase the branch on the current master, that would lead to a much cleaner commit history (I could also do the rebase, if that's easier for you)? Sorry for being unclear on what I had in mind to resolve the merge conflict. |
8 tasks
added 2 commits
April 23, 2020 10:04
Benchmark results for internal/repository: name old time/op new time/op delta LoadTree-8 479µs ± 2% 478µs ± 1% ~ (p=0.780 n=10+9) LoadBlob-8 11.6ms ± 2% 11.6ms ± 1% ~ (p=0.631 n=10+10) LoadAndDecrypt-8 13.2ms ± 2% 13.3ms ± 3% ~ (p=0.631 n=10+10) name old alloc/op new alloc/op delta LoadTree-8 41.2kB ± 0% 41.2kB ± 0% ~ (all equal) LoadBlob-8 2.28kB ± 0% 2.28kB ± 0% ~ (all equal) LoadAndDecrypt-8 2.10MB ± 0% 2.10MB ± 0% ~ (all equal) name old allocs/op new allocs/op delta LoadTree-8 652 ± 0% 652 ± 0% ~ (all equal) LoadBlob-8 24.0 ± 0% 24.0 ± 0% ~ (all equal) LoadAndDecrypt-8 30.0 ± 0% 30.0 ± 0% ~ (all equal) name old speed new speed delta LoadBlob-8 86.2MB/s ± 2% 86.4MB/s ± 1% ~ (p=0.594 n=10+10) LoadAndDecrypt-8 75.7MB/s ± 2% 75.4MB/s ± 3% ~ (p=0.617 n=10+10)
Pushing the allocation logic down into the former loadBlob body means that fewer allocations have to be performed: name old time/op new time/op delta LoadTree-8 478µs ± 1% 481µs ± 2% ~ (p=0.315 n=9+10) LoadBlob-8 11.6ms ± 1% 11.6ms ± 2% ~ (p=0.393 n=10+10) LoadAndDecrypt-8 13.3ms ± 3% 13.3ms ± 3% ~ (p=0.905 n=10+9) LoadIndex-8 33.6ms ± 2% 33.2ms ± 1% -1.15% (p=0.028 n=10+9) name old alloc/op new alloc/op delta LoadTree-8 41.2kB ± 0% 41.1kB ± 0% -0.23% (p=0.000 n=10+10) LoadBlob-8 2.28kB ± 0% 2.18kB ± 0% -4.21% (p=0.000 n=10+10) LoadAndDecrypt-8 2.10MB ± 0% 2.10MB ± 0% ~ (all equal) LoadIndex-8 5.22MB ± 0% 5.22MB ± 0% ~ (p=0.631 n=10+10) name old allocs/op new allocs/op delta LoadTree-8 652 ± 0% 651 ± 0% -0.15% (p=0.000 n=10+10) LoadBlob-8 24.0 ± 0% 23.0 ± 0% -4.17% (p=0.000 n=10+10) LoadAndDecrypt-8 30.0 ± 0% 30.0 ± 0% ~ (all equal) LoadIndex-8 30.2k ± 0% 30.2k ± 0% ~ (p=0.610 n=10+10) name old speed new speed delta LoadBlob-8 86.4MB/s ± 1% 85.9MB/s ± 2% ~ (p=0.393 n=10+10) LoadAndDecrypt-8 75.4MB/s ± 3% 75.4MB/s ± 3% ~ (p=0.858 n=10+9)
726cc87 to
e7d7b85
Compare
Contributor
Author
|
I was hesitant to rebase because the last time it triggered a re-review, but I did it now. |
Member
|
Merged, thanks! Github seems to have detected the rebase. Btw, your master branch seems to be slightly out of date. |
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
2 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.
What is the purpose of this change? What does it change?
Repository.LoadBlob currently requires its caller to compute the size of the buffer it needs. It then checks whether the buffer has the right size, and its helper loadBlob checks again. This leads to much duplicated, yet subtly different, code for computing sizes as well as repeated index lookups.
This PR changes LoadBlob to just return a freshly allocated buffer if insufficient space was passed in. The benchmarks show that fewer allocations have to be performed. The simplified code also makes it easier to add compression, since LoadBlob callers no longer need to know how big their buffers need to be (they'd have to know about compression as well as encryption).
There is one functional change, which is the error message from LoadTree will now say "id %v not found in repository" rather than "tree %v not found in repository".
Was the change discussed in an issue or in the forum before?
Closes #2629.
Checklist
gofmton the code in all commits