lib/model: Microoptimization of unifySubs and blockDiff

GitHub-Pull-Request: https://github.com/syncthing/syncthing/pull/4671
LGTM: AudriusButkevicius, calmh

lib/model/model.go

@@ -2781,48 +2781,30 @@ func readOffsetIntoBuf(fs fs.Filesystem, file string, offset int64, buf []byte)
 // The exists function is expected to return true for all known paths
 // (excluding "" and ".")
 func unifySubs(dirs []string, exists func(dir string) bool) []string {
-	subs := trimUntilParentKnown(dirs, exists)
+	if len(dirs) == 0 {
-	sort.Strings(subs)
-	return simplifySortedPaths(subs)
-}
-
-func trimUntilParentKnown(dirs []string, exists func(dir string) bool) []string {
-	var subs []string
-	for _, sub := range dirs {
-		for sub != "" && !fs.IsInternal(sub) {
-			sub = filepath.Clean(sub)
-			parent := filepath.Dir(sub)
-			if parent == "." || exists(parent) {
-				break
-			}
-			sub = parent
-			if sub == "." || sub == string(filepath.Separator) {
-				// Shortcut. We are going to scan the full folder, so we can
-				// just return an empty list of subs at this point.
 		return nil
 	}
-		}
+	sort.Strings(dirs)
-		if sub == "" {
+	if dirs[0] == "" || dirs[0] == "." || dirs[0] == string(fs.PathSeparator) {
 		return nil
 	}
-		subs = append(subs, sub)
+	prev := "./" // Anything that can't be parent of a clean path
+	for i := 0; i < len(dirs); {
+		dir := filepath.Clean(dirs[i])
+		if dir == prev || strings.HasPrefix(dir, prev+string(fs.PathSeparator)) {
+			dirs = append(dirs[:i], dirs[i+1:]...)
+			continue
 		}
-	return subs
+		parent := filepath.Dir(dir)
-}
+		for parent != "." && parent != string(fs.PathSeparator) && !exists(parent) {
-
+			dir = parent
-func simplifySortedPaths(subs []string) []string {
+			parent = filepath.Dir(dir)
-	var cleaned []string
-next:
-	for _, sub := range subs {
-		for _, existing := range cleaned {
-			if sub == existing || strings.HasPrefix(sub, existing+string(fs.PathSeparator)) {
-				continue next
 		}
+		dirs[i] = dir
+		prev = dir
+		i++
 	}
-		cleaned = append(cleaned, sub)
+	return dirs
-	}
-
-	return cleaned
 }
 
 // makeForgetUpdate takes an index update and constructs a download progress update

lib/model/model_test.go

@@ -2087,12 +2087,14 @@ func benchmarkTree(b *testing.B, n1, n2 int) {
 	b.ReportAllocs()
 }
 
-func TestUnifySubs(t *testing.T) {
+type unifySubsCase struct {
-	cases := []struct {
 	in     []string // input to unifySubs
 	exists []string // paths that exist in the database
 	out    []string // expected output
-	}{
+}
+
+func unifySubsCases() []unifySubsCase {
+	cases := []unifySubsCase{
 		{
 			// 0. trailing slashes are cleaned, known paths are just passed on
 			[]string{"foo/", "bar//"},
@@ -2174,16 +2176,24 @@ func TestUnifySubs(t *testing.T) {
 		}
 	}
 
-	for i, tc := range cases {
+	return cases
-		exists := func(f string) bool {
+}
+
+func unifyExists(f string, tc unifySubsCase) bool {
 	for _, e := range tc.exists {
 		if f == e {
 			return true
 		}
 	}
 	return false
-		}
+}
 
+func TestUnifySubs(t *testing.T) {
+	cases := unifySubsCases()
+	for i, tc := range cases {
+		exists := func(f string) bool {
+			return unifyExists(f, tc)
+		}
 		out := unifySubs(tc.in, exists)
 		if diff, equal := messagediff.PrettyDiff(tc.out, out); !equal {
 			t.Errorf("Case %d failed; got %v, expected %v, diff:\n%s", i, out, tc.out, diff)
@@ -2191,6 +2201,20 @@ func TestUnifySubs(t *testing.T) {
 	}
 }
 
+func BenchmarkUnifySubs(b *testing.B) {
+	cases := unifySubsCases()
+	b.ReportAllocs()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		for _, tc := range cases {
+			exists := func(f string) bool {
+				return unifyExists(f, tc)
+			}
+			unifySubs(tc.in, exists)
+		}
+	}
+}
+
 func TestIssue3028(t *testing.T) {
 	// Create two files that we'll delete, one with a name that is a prefix of the other.
 

lib/model/rwfolder.go

@@ -1032,9 +1032,9 @@ func (f *sendReceiveFolder) handleFile(file protocol.FileInfo, copyChan chan<- c
 
 	populateOffsets(file.Blocks)
 
-	var blocks []protocol.BlockInfo
+	blocks := make([]protocol.BlockInfo, 0, len(file.Blocks))
 	var blocksSize int64
-	var reused []int32
+	reused := make([]int32, 0, len(file.Blocks))
 
 	// Check for an old temporary file which might have some blocks we could
 	// reuse.
@@ -1127,18 +1127,24 @@ func (f *sendReceiveFolder) handleFile(file protocol.FileInfo, copyChan chan<- c
 
 // blockDiff returns lists of common and missing (to transform src into tgt)
 // blocks. Both block lists must have been created with the same block size.
-func blockDiff(src, tgt []protocol.BlockInfo) (have, need []protocol.BlockInfo) {
+func blockDiff(src, tgt []protocol.BlockInfo) ([]protocol.BlockInfo, []protocol.BlockInfo) {
-	if len(tgt) == 0 && len(src) != 0 {
+	if len(tgt) == 0 {
 		return nil, nil
 	}
 
-	if len(tgt) != 0 && len(src) == 0 {
+	if len(src) == 0 {
 		// Copy the entire file
 		return nil, tgt
 	}
 
+	have := make([]protocol.BlockInfo, 0, len(src))
+	need := make([]protocol.BlockInfo, 0, len(tgt))
+
 	for i := range tgt {
-		if i >= len(src) || !bytes.Equal(tgt[i].Hash, src[i].Hash) {
+		if i >= len(src) {
+			return have, append(need, tgt[i:]...)
+		}
+		if !bytes.Equal(tgt[i].Hash, src[i].Hash) {
 			// Copy differing block
 			need = append(need, tgt[i])
 		} else {

lib/model/rwfolder_test.go

@@ -692,6 +692,22 @@ func TestDiff(t *testing.T) {
 	}
 }
 
+func BenchmarkDiff(b *testing.B) {
+	testCases := make([]struct{ a, b []protocol.BlockInfo }, 0, len(diffTestData))
+	for _, test := range diffTestData {
+		a, _ := scanner.Blocks(context.TODO(), bytes.NewBufferString(test.a), test.s, -1, nil, false)
+		b, _ := scanner.Blocks(context.TODO(), bytes.NewBufferString(test.b), test.s, -1, nil, false)
+		testCases = append(testCases, struct{ a, b []protocol.BlockInfo }{a, b})
+	}
+	b.ReportAllocs()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		for _, tc := range testCases {
+			blockDiff(tc.a, tc.b)
+		}
+	}
+}
+
 func TestDiffEmpty(t *testing.T) {
 	emptyCases := []struct {
 		a    []protocol.BlockInfo