From 20aaa5927b982b7fdd01c173b58dc534f59aceee Mon Sep 17 00:00:00 2001 From: Jakob Borg Date: Tue, 10 Mar 2020 14:46:49 +0100 Subject: [PATCH] lib/protocol: Use BlocksHash to compare block lists when available (#6401) This is an optimization for faster equal checks on block lists. --- lib/model/folder_sendrecv.go | 4 +-- lib/protocol/bep_extensions.go | 17 +++++++++--- lib/protocol/protocol_test.go | 48 ++++++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 5 deletions(-) diff --git a/lib/model/folder_sendrecv.go b/lib/model/folder_sendrecv.go index dee389295..e76b5571d 100644 --- a/lib/model/folder_sendrecv.go +++ b/lib/model/folder_sendrecv.go @@ -364,7 +364,7 @@ func (f *sendReceiveFolder) processNeeded(snap *db.Snapshot, dbUpdateChan chan<- case file.Type == protocol.FileInfoTypeFile: curFile, hasCurFile := snap.Get(protocol.LocalDeviceID, file.Name) - if _, need := blockDiff(curFile.Blocks, file.Blocks); hasCurFile && len(need) == 0 { + if hasCurFile && file.BlocksEqual(curFile) { // We are supposed to copy the entire file, and then fetch nothing. We // are only updating metadata, so we don't actually *need* to make the // copy. @@ -460,7 +460,7 @@ nextFile: // we can just do a rename instead. key := string(fi.Blocks[0].Hash) for i, candidate := range buckets[key] { - if protocol.BlocksEqual(candidate.Blocks, fi.Blocks) { + if candidate.BlocksEqual(fi) { // Remove the candidate from the bucket lidx := len(buckets[key]) - 1 buckets[key][i] = buckets[key][lidx] diff --git a/lib/protocol/bep_extensions.go b/lib/protocol/bep_extensions.go index 0666ad9f3..bc0b41bae 100644 --- a/lib/protocol/bep_extensions.go +++ b/lib/protocol/bep_extensions.go @@ -216,7 +216,7 @@ func (f FileInfo) isEquivalent(other FileInfo, modTimeWindow time.Duration, igno switch f.Type { case FileInfoTypeFile: - return f.Size == other.Size && ModTimeEqual(f.ModTime(), other.ModTime(), modTimeWindow) && (ignoreBlocks || BlocksEqual(f.Blocks, other.Blocks)) + return f.Size == other.Size && ModTimeEqual(f.ModTime(), other.ModTime(), modTimeWindow) && (ignoreBlocks || f.BlocksEqual(other)) case FileInfoTypeSymlink: return f.SymlinkTarget == other.SymlinkTarget case FileInfoTypeDirectory: @@ -249,9 +249,20 @@ func PermsEqual(a, b uint32) bool { } } -// BlocksEqual returns whether two slices of blocks are exactly the same hash +// BlocksEqual returns true when the two files have identical block lists. +func (f FileInfo) BlocksEqual(other FileInfo) bool { + // If both sides have blocks hashes then we can just compare those. + if len(f.BlocksHash) > 0 && len(other.BlocksHash) > 0 { + return bytes.Equal(f.BlocksHash, other.BlocksHash) + } + + // Actually compare the block lists in full. + return blocksEqual(f.Blocks, other.Blocks) +} + +// blocksEqual returns whether two slices of blocks are exactly the same hash // and index pair wise. -func BlocksEqual(a, b []BlockInfo) bool { +func blocksEqual(a, b []BlockInfo) bool { if len(b) != len(a) { return false } diff --git a/lib/protocol/protocol_test.go b/lib/protocol/protocol_test.go index 5b4ebd9f5..9e71785b2 100644 --- a/lib/protocol/protocol_test.go +++ b/lib/protocol/protocol_test.go @@ -867,3 +867,51 @@ func TestDispatcherToCloseDeadlock(t *testing.T) { t.Fatal("timed out before dispatcher loop terminated") } } + +func TestBlocksEqual(t *testing.T) { + blocksOne := []BlockInfo{{Hash: []byte{1, 2, 3, 4}}} + blocksTwo := []BlockInfo{{Hash: []byte{5, 6, 7, 8}}} + hashOne := []byte{42, 42, 42, 42} + hashTwo := []byte{29, 29, 29, 29} + + cases := []struct { + b1 []BlockInfo + h1 []byte + b2 []BlockInfo + h2 []byte + eq bool + }{ + {blocksOne, hashOne, blocksOne, hashOne, true}, // everything equal + {blocksOne, hashOne, blocksTwo, hashTwo, false}, // nothing equal + {blocksOne, hashOne, blocksOne, nil, true}, // blocks compared + {blocksOne, nil, blocksOne, nil, true}, // blocks compared + {blocksOne, nil, blocksTwo, nil, false}, // blocks compared + {blocksOne, hashOne, blocksTwo, hashOne, true}, // hashes equal, blocks not looked at + {blocksOne, hashOne, blocksOne, hashTwo, false}, // hashes different, blocks not looked at + {blocksOne, hashOne, nil, nil, false}, // blocks is different from no blocks + {blocksOne, nil, nil, nil, false}, // blocks is different from no blocks + {nil, hashOne, nil, nil, true}, // nil blocks are equal, even of one side has a hash + } + + for _, tc := range cases { + f1 := FileInfo{Blocks: tc.b1, BlocksHash: tc.h1} + f2 := FileInfo{Blocks: tc.b2, BlocksHash: tc.h2} + + if !f1.BlocksEqual(f1) { + t.Error("f1 is always equal to itself", f1) + } + if !f2.BlocksEqual(f2) { + t.Error("f2 is always equal to itself", f2) + } + if res := f1.BlocksEqual(f2); res != tc.eq { + t.Log("f1", f1.BlocksHash, f1.Blocks) + t.Log("f2", f2.BlocksHash, f2.Blocks) + t.Errorf("f1.BlocksEqual(f2) == %v but should be %v", res, tc.eq) + } + if res := f2.BlocksEqual(f1); res != tc.eq { + t.Log("f1", f1.BlocksHash, f1.Blocks) + t.Log("f2", f2.BlocksHash, f2.Blocks) + t.Errorf("f2.BlocksEqual(f1) == %v but should be %v", res, tc.eq) + } + } +}