From d067a08dce0bc967a39e512c3980e930cc4cdc0c Mon Sep 17 00:00:00 2001 From: William Douglas Date: Mon, 6 Jan 2025 11:19:27 -0800 Subject: [PATCH] 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 --- swupd/delta.go | 4 ++-- swupd/delta_test.go | 26 +++++++++++++++++++++++--- swupd/files.go | 20 ++++++++++++++++++++ swupd/files_test.go | 37 +++++++++++++++++++++++++++++++++++++ swupd/helpers_test.go | 24 ++++++++++++++++-------- swupd/packs.go | 4 ++++ swupd/packs_test.go | 16 ++++++++++++---- 7 files changed, 114 insertions(+), 17 deletions(-) diff --git a/swupd/delta.go b/swupd/delta.go index 52b753d3c..2b4760462 100644 --- a/swupd/delta.go +++ b/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 } diff --git a/swupd/delta_test.go b/swupd/delta_test.go index c04112bff..e5070d4eb 100644 --- a/swupd/delta_test.go +++ b/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) +} diff --git a/swupd/files.go b/swupd/files.go index 4a2c42532..8124b7ef9 100644 --- a/swupd/files.go +++ b/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 +} diff --git a/swupd/files_test.go b/swupd/files_test.go index 37c8ff1cf..efef3aee1 100644 --- a/swupd/files_test.go +++ b/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 diff --git a/swupd/helpers_test.go b/swupd/helpers_test.go index f57d9db9c..dff530843 100644 --- a/swupd/helpers_test.go +++ b/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) } diff --git a/swupd/packs.go b/swupd/packs.go index ccc782ee8..8661fde33 100644 --- a/swupd/packs.go +++ b/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 diff --git a/swupd/packs_test.go b/swupd/packs_test.go index 6cb4a76bc..8f953dc9d 100644 --- a/swupd/packs_test.go +++ b/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) {