repository/index: Avoid creating an error when checking if an id is in the index. #1538
Conversation
There was a problem hiding this comment.
Hey, thanks for proposing the enhancemente! What I'd love to see is a benchmark showing the improvement... I can already imagine that creating the error takes CPU time (and memory), since we're using the github.com/pkg/errors library, which capture stack traces.
I'd like to propose a slightly different approach, which will use even less CPU time and memory: Undo your changes, then modify the Has() method to try the lookup in idx.pack directly (after locking of course). That will be even more efficient because bundling the locations where blobs are stored isn't needed.
What are your thoughts?
Codecov Report
@@ Coverage Diff @@
## master #1538 +/- ##
=========================================
- Coverage 51.55% 46.3% -5.25%
=========================================
Files 138 138
Lines 11080 11081 +1
=========================================
- Hits 5712 5131 -581
- Misses 4503 5139 +636
+ Partials 865 811 -54
Continue to review full report at Codecov.
|
|
Regarding the benchmark, I posted a note about the speed up I get running a backup of my system (saving about an hour of time in a 2-3 hour backup). Is there a more established way to do the benchmark? If it helps, I found this by doing some CPU profiling of my backup. Regarding your suggestion, that does sound great. If I understand correctly, I just have the one outer if statement? That sounds like it could help some more (lookupInternal still dominates the cpu profile). I'll give it a test here and see how it goes. |
|
Hey, for Go software it's very easy to write a micro benchmark of just the const maxPackSize = 16 * 1024 * 1024
func createRandomIndex() (idx *repository.Index, lookupID restic.ID) {
idx = repository.NewIndex()
// create index with 200k pack files
for i := 0; i < 200000; i++ {
packID := restic.NewRandomID()
offset := 0
for offset < maxPackSize {
size := 2000 + rand.Intn(4*1024*1024)
id := restic.NewRandomID()
idx.Store(restic.PackedBlob{
PackID: packID,
Blob: restic.Blob{
Type: restic.DataBlob,
ID: id,
Length: uint(size),
Offset: uint(offset),
},
})
offset += size
if rand.Float32() < 0.001 && lookupID.IsNull() {
lookupID = id
}
}
}
return idx, lookupID
}
func BenchmarkIndexHasUnknown(b *testing.B) {
idx, _ := createRandomIndex()
lookupID := restic.NewRandomID()
b.ResetTimer()
for i := 0; i < b.N; i++ {
idx.Has(lookupID, restic.DataBlob)
}
}
func BenchmarkIndexHasKnown(b *testing.B) {
idx, lookupID := createRandomIndex()
b.ResetTimer()
for i := 0; i < b.N; i++ {
idx.Has(lookupID, restic.DataBlob)
}
}You can then run just this benchmark and look at the memory allocation: So, we can see that each call of With your change cherry picked, this looks already much better: We can use With my suggestions, we can even improve further: And benchcmp agrees: 😄 So, if it's okay for you, I'd like to take over from here. Is that okay for you? Thank you very very much for pointing me to this improvement opportunity! |
MJDSys
force-pushed
the
make_lookup_internal
branch
from
ed01fca
to
063e883
Compare
|
Ah, I didn't realize you meant that kind of benchmark. Sorry about that. Feel free to take over this pr if you want. I pushed my changes to the Has function (undoing my other changes). I also whipped up a quick test for the Has function (though it seems thoroughly tested by all the other tests), so feel free to grab that as well. I have checked the "Allow Maintainers to make changes" button if you want to just add to this pr. I think there is still some optimizations available to get my backup to run faster, so I'll look into those as well. If I find any, I'll try to add a mirco-benchmark for those as well next time. |
|
Cool, thanks! I'll reformat the title of your commit, but otherwise leave it intact so that you get the credit for the change. Cheers! |
When backing up several million files (>14M tested here) with few changes, a large amount of time is spent failing to find an id in an index and creating an error to signify this. Since this is checked using the Has method, which doesn't use this error, this time creating the error is wasted. Instead, directly check if the given id and type are present in the index. This also avoids reporting all the packs containing this blob, further reducing cpu usage.
fd0
force-pushed
the
make_lookup_internal
branch
from
063e883
to
539599d
Compare
repository/index: Avoid creating an error when checking if an id is in the index.
|
Thanks to @fd0 and @MJDSys for so nicely working together in the way only OSS can enable to the benefit of us all. Nice improvement you found there and I would love to see more of these "little things" improved :). Looking at the speed of restic it's incredible to see that this can still be a 50% improvement on backup time! (see @fd0's comment). |
|
@MJDSys FYI, this PR cut my backup time on a larger server (~250GB data size, ~400GB repo size) in half: from ~45 min to ~20 min. Thanks again! |
What is the purpose of this change? What does it change?
When backing up several million files (>14M tested here) with few changes,
a large amount of time is spent failing to find an id in an index and creating
an error to signify this. Since this is checked using the Has method,
which doesn't use this error, this time creating the error is wasted.
Instead, create a new function lookupInternal that doesn't create this error.
Change Has to use this function and change Lookup to use lookupInternal and
create the error if it fails to find the id.
This results in saving about one hour of wall time (about half the backup
time) and about six hours of cpu time on my system.
I'm not sure if you want Lookup just changed to work with my lookupInternal, and then fixup all the reverse dependencies. If this is wanted, I'll update my PR appropriately. I also wasn't sure if you wanted a changelog entry, but I'm happy to add that too if wanted.
Was the change discussed in an issue or in the forum before?
No.
Checklist
changelog/x.y.zthat describe the changes for our users (template here)gofmton the code in all commits