From 6c98cd00f590abede4fa00da8e16eae5a1bedced Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Wed, 7 Aug 2019 11:53:53 +0100 Subject: [PATCH] Only calculate container fields that are requested The ListImagesWithOptions API method lets you supply a list of the fields you want for each container, so that you can cut down on the size of the response when you don't care about some fields. But all the fields are calculated, whatever you ask for, which involves a lot of sorting and filtering. This commit makes the procedure calculating the fields _only_ calculate the fields that were actually requested. Some of the calculations depend on the same intermediate results; so, the approach is to lazily calculate and cache intermediate results. --- api/v6/container.go | 152 ++++++++++++++++++++++++--------------- api/v6/container_test.go | 102 +++++++++++--------------- 2 files changed, 136 insertions(+), 118 deletions(-) diff --git a/api/v6/container.go b/api/v6/container.go index 5fd0dc1543..46d7ccb862 100644 --- a/api/v6/container.go +++ b/api/v6/container.go @@ -28,52 +28,6 @@ type Container struct { // NewContainer creates a Container given a list of images and the current image func NewContainer(name string, images []image.Info, currentImage image.Info, tagPattern policy.Pattern, fields []string) (Container, error) { - sorted := update.SortImages(images, tagPattern) - - // All images - imagesCount := len(sorted) - imagesErr := "" - if sorted == nil { - imagesErr = registry.ErrNoImageData.Error() - } - var newImages update.SortedImageInfos - for _, img := range sorted { - if tagPattern.Newer(&img, ¤tImage) { - newImages = append(newImages, img) - } - } - newImagesCount := len(newImages) - - // Filtered images (which respects sorting) - filteredImages := update.SortedImageInfos(update.FilterImages(sorted, tagPattern)) - filteredImagesCount := len(filteredImages) - var newFilteredImages update.SortedImageInfos - for _, img := range filteredImages { - if tagPattern.Newer(&img, ¤tImage) { - newFilteredImages = append(newFilteredImages, img) - } - } - newFilteredImagesCount := len(newFilteredImages) - latestFiltered, _ := filteredImages.Latest() - - container := Container{ - Name: name, - Current: currentImage, - LatestFiltered: latestFiltered, - - Available: sorted, - AvailableError: imagesErr, - AvailableImagesCount: imagesCount, - NewAvailableImagesCount: newImagesCount, - FilteredImagesCount: filteredImagesCount, - NewFilteredImagesCount: newFilteredImagesCount, - } - return filterContainerFields(container, fields) -} - -// filterContainerFields returns a new container with only the fields specified. If not fields are specified, -// a list of default fields is used. -func filterContainerFields(container Container, fields []string) (Container, error) { // Default fields if len(fields) == 0 { fields = []string{ @@ -90,29 +44,109 @@ func filterContainerFields(container Container, fields []string) (Container, err } var c Container + + // The following machinery attempts to minimise the number of + // filters (`O(n)`) and sorts (`O(n log n)`), by memoising and + // sharing intermediate results. + + var ( + sortedImages update.SortedImageInfos + filteredImages []image.Info + sortedFilteredImages update.SortedImageInfos + ) + + getSortedImages := func() update.SortedImageInfos { + if sortedImages == nil { + sortedImages = update.SortImages(images, tagPattern) + } + return sortedImages + } + + getFilteredImages := func() []image.Info { + if filteredImages == nil { + filteredImages = update.FilterImages(images, tagPattern) + } + return filteredImages + } + + getSortedFilteredImages := func() update.SortedImageInfos { + if sortedFilteredImages == nil { + sortedFilteredImages = update.SortImages(getFilteredImages(), tagPattern) + } + return sortedFilteredImages + } + + assignFields := []func(){} + for _, field := range fields { switch field { + // these first few rely only on the inputs case "Name": - c.Name = container.Name + c.Name = name case "Current": - c.Current = container.Current - case "LatestFiltered": - c.LatestFiltered = container.LatestFiltered - case "Available": - c.Available = container.Available + c.Current = currentImage case "AvailableError": - c.AvailableError = container.AvailableError + if images == nil { + c.AvailableError = registry.ErrNoImageData.Error() + } case "AvailableImagesCount": - c.AvailableImagesCount = container.AvailableImagesCount + c.AvailableImagesCount = len(images) + + case "Available": // needs sorted images + assignFields = append(assignFields, func() { + c.Available = getSortedImages() + }) + // now that we know we need to sort all the images anyway, + // the fastest way to get sorted, filtered images will be + // to filter the already sorted images + getSortedFilteredImages = func() update.SortedImageInfos { + return update.FilterImages(getSortedImages(), tagPattern) + } + getFilteredImages = func() []image.Info { + return []image.Info(getSortedFilteredImages()) + } + + case "LatestFiltered": // needs sorted, filtered images + assignFields = append(assignFields, func() { + latest, _ := getSortedFilteredImages().Latest() + c.LatestFiltered = latest + }) + case "NewAvailableImagesCount": - c.NewAvailableImagesCount = container.NewAvailableImagesCount - case "FilteredImagesCount": - c.FilteredImagesCount = container.FilteredImagesCount - case "NewFilteredImagesCount": - c.NewFilteredImagesCount = container.NewFilteredImagesCount + assignFields = append(assignFields, func() { + newImagesCount := 0 + for _, img := range getSortedImages() { + if !tagPattern.Newer(&img, ¤tImage) { + break + } + newImagesCount++ + } + c.NewAvailableImagesCount = newImagesCount + }) + + case "FilteredImagesCount": // needs filtered tags + assignFields = append(assignFields, func() { + c.FilteredImagesCount = len(getFilteredImages()) + }) + case "NewFilteredImagesCount": // needs filtered images + assignFields = append(assignFields, func() { + newFilteredImagesCount := 0 + for _, img := range getSortedFilteredImages() { + if !tagPattern.Newer(&img, ¤tImage) { + break + } + newFilteredImagesCount++ + } + c.NewFilteredImagesCount = newFilteredImagesCount + }) default: return c, errors.Errorf("%s is an invalid field", field) } } + + for _, fn := range assignFields { + fn() + } + return c, nil } diff --git a/api/v6/container_test.go b/api/v6/container_test.go index cb77e388ca..fb1be24dd6 100644 --- a/api/v6/container_test.go +++ b/api/v6/container_test.go @@ -1,7 +1,6 @@ package v6 import ( - "reflect" "testing" "github.com/stretchr/testify/assert" @@ -71,81 +70,66 @@ func TestNewContainer(t *testing.T) { }, wantErr: false, }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := NewContainer(tt.args.name, tt.args.images, tt.args.currentImage, tt.args.tagPattern, tt.args.fields) - assert.Equal(t, tt.wantErr, err != nil) - assert.Equal(t, tt.want, got) - }) - } -} - -func TestFilterContainerFields(t *testing.T) { - testContainer := Container{ - Name: "test", - Current: image.Info{ImageID: "123"}, - LatestFiltered: image.Info{ImageID: "123"}, - Available: []image.Info{{ImageID: "123"}}, - AvailableError: "test", - AvailableImagesCount: 1, - NewAvailableImagesCount: 2, - FilteredImagesCount: 3, - NewFilteredImagesCount: 4, - } - - type args struct { - container Container - fields []string - } - tests := []struct { - name string - args args - want Container - wantErr bool - }{ { - name: "Default fields", + name: "Require only some calculations", args: args{ - container: testContainer, + name: "container-some", + images: []image.Info{currentSemver, newSemver, oldSemver, testImage}, + currentImage: currentSemver, + tagPattern: policy.NewPattern("semver:*"), + fields: []string{"Name", "NewFilteredImagesCount"}, // but not, e.g., "FilteredImagesCount" + }, + want: Container{ + Name: "container-some", + NewFilteredImagesCount: 1, }, - want: testContainer, - wantErr: false, }, { - name: "FilterImages", + name: "Fields in one order", args: args{ - container: testContainer, - fields: []string{"Name", "Available", "NewAvailableImagesCount", "NewFilteredImagesCount"}, + name: "container-ordered1", + images: []image.Info{currentSemver, newSemver, oldSemver, testImage}, + currentImage: currentSemver, + tagPattern: policy.NewPattern("semver:*"), + fields: []string{"Name", + "AvailableImagesCount", "Available", // these two both depend on the same intermediate result + "LatestFiltered", "FilteredImagesCount", // these two both depend on another intermediate result + }, }, want: Container{ - Name: "test", - Available: []image.Info{{ImageID: "123"}}, - NewAvailableImagesCount: 2, - NewFilteredImagesCount: 4, + Name: "container-ordered1", + Available: []image.Info{newSemver, currentSemver, oldSemver, testImage}, + AvailableImagesCount: 4, + LatestFiltered: newSemver, + FilteredImagesCount: 3, }, - wantErr: false, }, { - name: "Invalid field", + name: "Fields in another order", args: args{ - container: testContainer, - fields: []string{"Invalid"}, + name: "container-ordered2", + images: []image.Info{currentSemver, newSemver, oldSemver, testImage}, + currentImage: currentSemver, + tagPattern: policy.NewPattern("semver:*"), + fields: []string{"Name", + "Available", "AvailableImagesCount", // these two latter depend on the same intermediate result, as above + "FilteredImagesCount", "LatestFiltered", // as above, similarly + }, + }, + want: Container{ + Name: "container-ordered2", + Available: []image.Info{newSemver, currentSemver, oldSemver, testImage}, + AvailableImagesCount: 4, + LatestFiltered: newSemver, + FilteredImagesCount: 3, }, - want: Container{}, - wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := filterContainerFields(tt.args.container, tt.args.fields) - if (err != nil) != tt.wantErr { - t.Errorf("FilterContainerFields() error = %v, wantErr %v", err, tt.wantErr) - return - } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("FilterContainerFields() = %v, want %v", got, tt.want) - } + got, err := NewContainer(tt.args.name, tt.args.images, tt.args.currentImage, tt.args.tagPattern, tt.args.fields) + assert.Equal(t, tt.wantErr, err != nil) + assert.Equal(t, tt.want, got) }) } }