Limit the amount of certain modifiers in packs

Systems using certain modifiers are currently not used as frequently
so in order to reduce pack size and improve update times skip those
modifiers from being considered for packs.

Currently the SSE version is skipped when either AVX2 or AVX512
versions exist and APX is always skipped.

This change does not apply to 0 packs as it would break installer
offline support.

Signed-off-by: William Douglas <william.douglas@intel.com>

swupd/delta.go

@@ -315,7 +315,7 @@ func findDeltas(c *config, oldManifest, newManifest *Manifest) ([]Delta, error)
 
 	deltaCount := 0
 	for _, nf := range newManifest.Files {
-		if nf.DeltaPeer != nil {
+		if nf.DeltaPeer != nil && nf.useInPack() {
 			deltaCount++
 		}
 	}
@@ -327,7 +327,7 @@ func findDeltas(c *config, oldManifest, newManifest *Manifest) ([]Delta, error)
 	seen := make(map[string]bool)
 
 	for _, nf := range newManifest.Files {
-		if nf.DeltaPeer == nil {
+		if nf.DeltaPeer == nil || !nf.useInPack() {
 			continue
 		}
 

swupd/delta_test.go

@@ -22,7 +22,7 @@ func TestCreateDeltas(t *testing.T) {
 	mustMkdir(t, filepath.Join(ts.Dir, "www/20/delta"))
 
 	mustCreateAllDeltas(t, "Manifest.full", ts.Dir, 10, 20)
-	mustExistDelta(t, ts.Dir, "/bar", 10, 20)
+	mustExistDelta(t, ts.Dir, "/bar", Sse0, 10, 20)
 }
 
 func TestCreateDeltaTooBig(t *testing.T) {
@@ -41,7 +41,7 @@ func TestCreateDeltaTooBig(t *testing.T) {
 	mustMkdir(t, filepath.Join(ts.Dir, "www/20/delta"))
 
 	tryCreateAllDeltas(t, "Manifest.full", ts.Dir, 10, 20)
-	mustNotExistDelta(t, ts.Dir, "/foo", 10, 20)
+	mustNotExistDelta(t, ts.Dir, "/foo", Sse0, 10, 20)
 }
 
 func TestCreateDeltaFULLDL(t *testing.T) {
@@ -61,7 +61,7 @@ m,cvnxcpowertw54lsi8ydoprf g,mdbng.c,mvnxb,.mxhstu;lwey5o;sdfjklgx;cnvjnxbasdfh`
 	mustMkdir(t, filepath.Join(ts.Dir, "www/20/delta"))
 
 	tryCreateAllDeltas(t, "Manifest.full", ts.Dir, 10, 20)
-	mustNotExistDelta(t, ts.Dir, "/foo", 10, 20)
+	mustNotExistDelta(t, ts.Dir, "/foo", Sse0, 10, 20)
 }
 
 // Imported from swupd-server/test/functional/no-delta.
@@ -146,3 +146,23 @@ func TestNoDeltasForTypeChangesOrDereferencedSymlinks(t *testing.T) {
 		t.Fatalf("found %d files in %s but expected %d", len(fis), ts.path("www/20/delta"), info.DeltaCount)
 	}
 }
+
+func TestOnlyUseInPackDeltas(t *testing.T) {
+	ts := newTestSwupd(t, "deltas")
+	defer ts.cleanup()
+	ts.Bundles = []string{"test-bundle1", "test-bundle2"}
+	ts.addFile(10, "test-bundle1", "/foo", strings.Repeat("foo", 100))
+	ts.addFile(10, "test-bundle2", "/bar", strings.Repeat("bar", 100))
+	ts.addFile(10, "test-bundle2", "/V3/bar", strings.Repeat("V3bar", 100))
+	ts.createManifests(10)
+
+	ts.addFile(20, "test-bundle1", "/foo", strings.Repeat("foo", 100))
+	ts.addFile(20, "test-bundle2", "/bar", strings.Repeat("bar", 100)+"testingdelta")
+	ts.addFile(20, "test-bundle2", "/V3/bar", strings.Repeat("V3bar", 100)+"testingdelta")
+	ts.createManifests(20)
+	mustMkdir(t, filepath.Join(ts.Dir, "www/20/delta"))
+
+	mustCreateAllDeltas(t, "Manifest.full", ts.Dir, 10, 20)
+	mustNotExistDelta(t, ts.Dir, "/bar", Sse1, 10, 20)
+	mustExistDelta(t, ts.Dir, "/V3/bar", Avx2_1, 10, 20)
+}

swupd/files.go

@@ -459,6 +459,16 @@ func (f *File) findFileNameInSlice(fs []*File) *File {
 	return nil
 }
 
+func (f *File) findFileNameAndModifierInSlice(fs []*File) *File {
+	for _, file := range fs {
+		if file.Name == f.Name && file.Modifier == f.Modifier {
+			return file
+		}
+	}
+
+	return nil
+}
+
 func (f *File) isUnsupportedTypeChange() bool {
 	if f.DeltaPeer == nil {
 		// nothing to check, new or deleted file
@@ -489,3 +499,13 @@ func (f *File) Present() bool {
 func (f *File) hasOptPrefix() bool {
 	return strings.HasPrefix(f.Name, "/V3") || strings.HasPrefix(f.Name, "/V4") || strings.HasPrefix(f.Name, "/VA")
 }
+
+func (f *File) useInPack() bool {
+	// At this point SSE files that also have an AVX version
+	// aren't going to be added to packs, simalarly, APX systems
+	// aren't popular enough yet to add to packs
+	var sseWithAvxOptBin = f.Modifier == Sse1 || f.Modifier == Sse2 || f.Modifier == Sse3
+	var apx = f.Modifier == Apx4 || f.Modifier == Apx5 || f.Modifier == Apx6 || f.Modifier == Apx7
+	var useFile = !apx && !sseWithAvxOptBin
+	return useFile
+}

swupd/files_test.go

@@ -314,6 +314,43 @@ func TestFindFileNameInSlice(t *testing.T) {
 	}
 }
 
+func TestFindFileNameAndModifierInSlice(t *testing.T) {
+	fs := []*File{
+		{Name: "1", Modifier: Sse0},
+		{Name: "2", Modifier: Avx2_1},
+		{Name: "3", Modifier: Apx7},
+	}
+
+	testCases := []struct {
+		name        string
+		modifier    ModifierFlag
+		hasMatch    bool
+		expectedIdx int
+	}{
+		{"1", Sse0, true, 0},
+		{"2", Avx2_1, true, 1},
+		{"3", Apx7, true, 2},
+		{"1", Sse1, false, 9},
+		{"2", Avx2_3, false, 9},
+		{"3", Apx4, false, 9},
+		{"4", Sse0, false, 9},
+		{"notpresent", Avx512_2, false, 9},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			f := File{Name: tc.name}
+			found := f.findFileNameInSlice(fs)
+			if tc.hasMatch {
+				if found.Name != fs[tc.expectedIdx].Name {
+					t.Errorf("findFileNameInSlice returned %v when %v expected",
+						found.Name, fs[tc.expectedIdx].Name)
+				}
+			}
+		})
+	}
+}
+
 func TestTypeHasChanged(t *testing.T) {
 	testCases := []struct {
 		file     File

swupd/helpers_test.go

@@ -124,7 +124,7 @@ func mustExist(t *testing.T, name string) {
 	}
 }
 
-func existDelta(t *testing.T, testDir, filename string, from, to uint32) string {
+func existDelta(t *testing.T, testDir, filename string, modifier ModifierFlag, from, to uint32) string {
 	t.Helper()
 	var fromFull *Manifest
 	var toFull *Manifest
@@ -136,22 +136,30 @@ func existDelta(t *testing.T, testDir, filename string, from, to uint32) string
 		t.Fatalf("Failed to load to manifest to read hash from: %q", err)
 	}
 
-	var fileNeeded = &File{Name: filename}
-	fromHash := fileNeeded.findFileNameInSlice(fromFull.Files).Hash
-	toHash := fileNeeded.findFileNameInSlice(toFull.Files).Hash
+	var fileNeeded = &File{Name: filename, Modifier: modifier}
+	fromFile := fileNeeded.findFileNameAndModifierInSlice(fromFull.Files)
+	if fromFile == nil {
+		t.Fatal("Failed to find file in from manifest")
+	}
+	fromHash := fromFile.Hash
+	toFile := fileNeeded.findFileNameAndModifierInSlice(toFull.Files)
+	if toFile == nil {
+		t.Fatal("Failed to find file in to manifest")
+	}
+	toHash := toFile.Hash
 
 	suffix := fmt.Sprintf("%d-%d-%s-%s", from, to, fromHash, toHash)
 	deltafile := filepath.Join(testDir, "www", fmt.Sprintf("%d", to), "delta", suffix)
 	return deltafile
 }
 
-func mustExistDelta(t *testing.T, testDir, filename string, from, to uint32) {
-	deltafile := existDelta(t, testDir, filename, from, to)
+func mustExistDelta(t *testing.T, testDir, filename string, modifier ModifierFlag, from, to uint32) {
+	deltafile := existDelta(t, testDir, filename, modifier, from, to)
 	mustExist(t, deltafile)
 }
 
-func mustNotExistDelta(t *testing.T, testDir, filename string, from, to uint32) {
-	deltafile := existDelta(t, testDir, filename, from, to)
+func mustNotExistDelta(t *testing.T, testDir, filename string, modifier ModifierFlag, from, to uint32) {
+	deltafile := existDelta(t, testDir, filename, modifier, from, to)
 	mustNotExist(t, deltafile)
 }
 

swupd/packs.go

@@ -237,6 +237,10 @@ func WritePack(w io.Writer, fromManifest, toManifest *Manifest, outputDir, chroo
 		entry := &info.Entries[i]
 		entry.File = f
 
+		if fromVersion != 0 && !f.useInPack() {
+			entry.Reason = "skipping file type of already included file"
+			continue
+		}
 		if f.Version <= fromVersion || fileContentInManifest(f, fromManifest) {
 			entry.Reason = "already in from manifest"
 			continue

swupd/packs_test.go

@@ -200,6 +200,7 @@ func TestCreatePackZeroPacks(t *testing.T) {
 	ts.addFile(10, "shells", "/csh", "csh contents")
 	ts.addFile(10, "shells", "/fish", "fish contents")
 	ts.addFile(10, "shells", "/zsh", "zsh contents")
+	ts.addFile(10, "shells", "/V3/zsh", "zsh V3 contents")
 
 	ts.createManifests(10)
 
@@ -212,7 +213,7 @@ func TestCreatePackZeroPacks(t *testing.T) {
 
 	info = ts.createPack("shells", 0, 10, ts.path("image"))
 	mustHaveNoWarnings(t, info)
-	mustHaveFullfileCount(t, info, 4+emptyFile)
+	mustHaveFullfileCount(t, info, 5+emptyFile)
 	mustHaveDeltaCount(t, info, 0)
 	mustValidateZeroPack(t, ts.path("www/10/Manifest.shells"), ts.path("www/10/pack-shells-from-0.tar"))
 
@@ -224,6 +225,7 @@ func TestCreatePackZeroPacks(t *testing.T) {
 	ts.addFile(20, "shells", "/csh", "csh contents")
 	ts.addFile(20, "shells", "/fish", "fish contents")
 	ts.addFile(20, "shells", "/zsh", "zsh contents")
+	ts.addFile(20, "shells", "/V3/zsh", "zsh V3 contents")
 	ts.addFile(20, "shells", "/ksh", "ksh contents")
 	ts.createManifests(20)
 
@@ -252,7 +254,7 @@ func TestCreatePackZeroPacks(t *testing.T) {
 	// Now we have all fullfiles for both versions.
 	info = ts.createPack("shells", 0, 20, "")
 	mustHaveNoWarnings(t, info)
-	mustHaveFullfileCount(t, info, 5+emptyFile)
+	mustHaveFullfileCount(t, info, 6+emptyFile)
 	mustHaveDeltaCount(t, info, 0)
 	mustValidateZeroPack(t, ts.path("www/20/Manifest.shells"), ts.path("www/20/pack-shells-from-0.tar"))
 }
@@ -342,6 +344,8 @@ func TestCreatePackWithDelta(t *testing.T) {
 	fs.write("image/10/contents/small2", smallContents)
 	fs.write("image/10/contents/large1", largeContents)
 	fs.write("image/10/contents/large2", largeContents)
+	fs.write("image/10/contents/opttest", largeContents+"0opt")
+	fs.write("image/10/contents/V3/opttest", largeContents+"V3opt")
 	mustCreateManifests(t, 10, 0, minVer, format, fs.Dir)
 
 	//
@@ -353,10 +357,14 @@ func TestCreatePackWithDelta(t *testing.T) {
 	fs.write("image/20/contents/small2", smallContents)
 	fs.write("image/20/contents/large1", strings.ToUpper(largeContents[:1])+largeContents[1:])
 	fs.write("image/20/contents/large2", largeContents[:1]+strings.ToUpper(largeContents[1:]))
+	fs.write("image/20/contents/opttest", largeContents+"0OPT")
+	fs.write("image/20/contents/V3/opttest", largeContents+"V3OPT")
+
 	mustCreateManifests(t, 20, 10, minVer, format, fs.Dir)
 
 	info := mustCreatePack(t, "contents", 10, 20, fs.path("www"), fs.path("image"))
-	mustHaveDeltaCount(t, info, 2)
+	// 3 not 4 as the non-V3 opttest shouldn't have a delta added
+	mustHaveDeltaCount(t, info, 3)
 
 	//
 	// In version 30, make a change to one large files from 20.
@@ -372,7 +380,7 @@ func TestCreatePackWithDelta(t *testing.T) {
 
 	// Pack between 10 and 30 has both deltas.
 	info = mustCreatePack(t, "contents", 10, 30, fs.path("www"), fs.path("image"))
-	mustHaveDeltaCount(t, info, 2)
+	mustHaveDeltaCount(t, info, 3)
 }
 
 func TestCreatePackWithIncompleteChrootDir(t *testing.T) {