From 624a0ab0b7071360a9f9dfa8a8eeae16cfef4648 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Rod=C3=A1k?= Date: Tue, 8 Oct 2024 18:39:16 +0200 Subject: [PATCH 1/2] Refactor operations with slices MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes duplicate copy*Slice functions using a generic copy function or replaces them with the slices.Clone function. Also simplifies the stringSliceWithoutValue function. These changes should not change the behavior. Signed-off-by: Jan Rodák --- containers.go | 14 +++--- images.go | 14 +++--- layers.go | 18 ++++---- store.go | 125 +++++++++++++------------------------------------- 4 files changed, 54 insertions(+), 117 deletions(-) diff --git a/containers.go b/containers.go index 5ddd56e8f2..661fa6e314 100644 --- a/containers.go +++ b/containers.go @@ -164,16 +164,16 @@ type containerStore struct { func copyContainer(c *Container) *Container { return &Container{ ID: c.ID, - Names: copyStringSlice(c.Names), + Names: copySlicePreferringNil(c.Names), ImageID: c.ImageID, LayerID: c.LayerID, Metadata: c.Metadata, - BigDataNames: copyStringSlice(c.BigDataNames), + BigDataNames: copySlicePreferringNil(c.BigDataNames), BigDataSizes: maps.Clone(c.BigDataSizes), BigDataDigests: maps.Clone(c.BigDataDigests), Created: c.Created, - UIDMap: copyIDMap(c.UIDMap), - GIDMap: copyIDMap(c.GIDMap), + UIDMap: copySlicePreferringNil(c.UIDMap), + GIDMap: copySlicePreferringNil(c.GIDMap), Flags: maps.Clone(c.Flags), volatileStore: c.volatileStore, } @@ -693,8 +693,8 @@ func (r *containerStore) create(id string, names []string, image, layer string, BigDataDigests: make(map[string]digest.Digest), Created: time.Now().UTC(), Flags: copyStringInterfaceMap(options.Flags), - UIDMap: copyIDMap(options.UIDMap), - GIDMap: copyIDMap(options.GIDMap), + UIDMap: copySlicePreferringNil(options.UIDMap), + GIDMap: copySlicePreferringNil(options.GIDMap), volatileStore: options.Volatile, } if options.MountOpts != nil { @@ -906,7 +906,7 @@ func (r *containerStore) BigDataNames(id string) ([]string, error) { if !ok { return nil, ErrContainerUnknown } - return copyStringSlice(c.BigDataNames), nil + return copySlicePreferringNil(c.BigDataNames), nil } // Requires startWriting. diff --git a/images.go b/images.go index 2e12967931..7ea5b5fb6e 100644 --- a/images.go +++ b/images.go @@ -183,13 +183,13 @@ func copyImage(i *Image) *Image { return &Image{ ID: i.ID, Digest: i.Digest, - Digests: copyDigestSlice(i.Digests), - Names: copyStringSlice(i.Names), - NamesHistory: copyStringSlice(i.NamesHistory), + Digests: copySlicePreferringNil(i.Digests), + Names: copySlicePreferringNil(i.Names), + NamesHistory: copySlicePreferringNil(i.NamesHistory), TopLayer: i.TopLayer, - MappedTopLayers: copyStringSlice(i.MappedTopLayers), + MappedTopLayers: copySlicePreferringNil(i.MappedTopLayers), Metadata: i.Metadata, - BigDataNames: copyStringSlice(i.BigDataNames), + BigDataNames: copySlicePreferringNil(i.BigDataNames), BigDataSizes: maps.Clone(i.BigDataSizes), BigDataDigests: maps.Clone(i.BigDataDigests), Created: i.Created, @@ -718,7 +718,7 @@ func (r *imageStore) create(id string, names []string, layer string, options Ima Digest: options.Digest, Digests: dedupeDigests(options.Digests), Names: names, - NamesHistory: copyStringSlice(options.NamesHistory), + NamesHistory: copySlicePreferringNil(options.NamesHistory), TopLayer: layer, Metadata: options.Metadata, BigDataNames: []string{}, @@ -967,7 +967,7 @@ func (r *imageStore) BigDataNames(id string) ([]string, error) { if !ok { return nil, fmt.Errorf("locating image with ID %q: %w", id, ErrImageUnknown) } - return copyStringSlice(image.BigDataNames), nil + return copySlicePreferringNil(image.BigDataNames), nil } // Requires startWriting. diff --git a/layers.go b/layers.go index 7491ce00ff..504ac08f6d 100644 --- a/layers.go +++ b/layers.go @@ -436,7 +436,7 @@ func layerLocation(l *Layer) layerLocations { func copyLayer(l *Layer) *Layer { return &Layer{ ID: l.ID, - Names: copyStringSlice(l.Names), + Names: copySlicePreferringNil(l.Names), Parent: l.Parent, Metadata: l.Metadata, MountLabel: l.MountLabel, @@ -451,12 +451,12 @@ func copyLayer(l *Layer) *Layer { CompressionType: l.CompressionType, ReadOnly: l.ReadOnly, volatileStore: l.volatileStore, - BigDataNames: copyStringSlice(l.BigDataNames), + BigDataNames: copySlicePreferringNil(l.BigDataNames), Flags: maps.Clone(l.Flags), - UIDMap: copyIDMap(l.UIDMap), - GIDMap: copyIDMap(l.GIDMap), - UIDs: copyUint32Slice(l.UIDs), - GIDs: copyUint32Slice(l.GIDs), + UIDMap: copySlicePreferringNil(l.UIDMap), + GIDMap: copySlicePreferringNil(l.GIDMap), + UIDs: copySlicePreferringNil(l.UIDs), + GIDs: copySlicePreferringNil(l.GIDs), } } @@ -1404,8 +1404,8 @@ func (r *layerStore) create(id string, parentLayer *Layer, names []string, mount UIDs: templateUIDs, GIDs: templateGIDs, Flags: copyStringInterfaceMap(moreOptions.Flags), - UIDMap: copyIDMap(moreOptions.UIDMap), - GIDMap: copyIDMap(moreOptions.GIDMap), + UIDMap: copySlicePreferringNil(moreOptions.UIDMap), + GIDMap: copySlicePreferringNil(moreOptions.GIDMap), BigDataNames: []string{}, volatileStore: moreOptions.Volatile, } @@ -1840,7 +1840,7 @@ func (r *layerStore) BigDataNames(id string) ([]string, error) { if !ok { return nil, fmt.Errorf("locating layer with ID %q to retrieve bigdata names: %w", id, ErrImageUnknown) } - return copyStringSlice(layer.BigDataNames), nil + return copySlicePreferringNil(layer.BigDataNames), nil } // Requires startReading or startWriting. diff --git a/store.go b/store.go index 8f33ea217c..441f070509 100644 --- a/store.go +++ b/store.go @@ -877,8 +877,8 @@ func GetStore(options types.StoreOptions) (Store, error) { graphOptions: options.GraphDriverOptions, imageStoreDir: options.ImageStore, pullOptions: options.PullOptions, - uidMap: copyIDMap(options.UIDMap), - gidMap: copyIDMap(options.GIDMap), + uidMap: copySlicePreferringNil(options.UIDMap), + gidMap: copySlicePreferringNil(options.GIDMap), autoUsernsUser: options.RootAutoNsUser, autoNsMinSize: autoNsMinSize, autoNsMaxSize: autoNsMaxSize, @@ -897,30 +897,6 @@ func GetStore(options types.StoreOptions) (Store, error) { return s, nil } -func copyUint32Slice(slice []uint32) []uint32 { - m := []uint32{} - if slice != nil { - m = make([]uint32, len(slice)) - copy(m, slice) - } - if len(m) > 0 { - return m[:] - } - return nil -} - -func copyIDMap(idmap []idtools.IDMap) []idtools.IDMap { - m := []idtools.IDMap{} - if idmap != nil { - m = make([]idtools.IDMap, len(idmap)) - copy(m, idmap) - } - if len(m) > 0 { - return m[:] - } - return nil -} - func (s *store) RunRoot() string { return s.runRoot } @@ -952,11 +928,11 @@ func (s *store) PullOptions() map[string]string { } func (s *store) UIDMap() []idtools.IDMap { - return copyIDMap(s.uidMap) + return copySlicePreferringNil(s.uidMap) } func (s *store) GIDMap() []idtools.IDMap { - return copyIDMap(s.gidMap) + return copySlicePreferringNil(s.gidMap) } // This must only be called when constructing store; it writes to fields that are assumed to be constant after construction. @@ -1469,7 +1445,7 @@ func (s *store) putLayer(rlstore rwLayerStore, rlstores []roLayerStore, id, pare var options LayerOptions if lOptions != nil { options = *lOptions - options.BigData = copyLayerBigDataOptionSlice(lOptions.BigData) + options.BigData = slices.Clone(lOptions.BigData) options.Flags = maps.Clone(lOptions.Flags) } if options.HostUIDMapping { @@ -1541,8 +1517,8 @@ func (s *store) putLayer(rlstore rwLayerStore, rlstores []roLayerStore, id, pare options.IDMappingOptions = types.IDMappingOptions{ HostUIDMapping: options.HostUIDMapping, HostGIDMapping: options.HostGIDMapping, - UIDMap: copyIDMap(uidMap), - GIDMap: copyIDMap(gidMap), + UIDMap: copySlicePreferringNil(uidMap), + GIDMap: copySlicePreferringNil(gidMap), } } return rlstore.create(id, parentLayer, names, mountLabel, nil, &options, writeable, diff, slo) @@ -1610,8 +1586,8 @@ func (s *store) CreateImage(id string, names []string, layer, metadata string, i Metadata: i.Metadata, CreationDate: i.Created, Digest: i.Digest, - Digests: copyDigestSlice(i.Digests), - NamesHistory: copyStringSlice(i.NamesHistory), + Digests: copySlicePreferringNil(i.Digests), + NamesHistory: copySlicePreferringNil(i.NamesHistory), } for _, key := range i.BigDataNames { data, err := store.BigData(id, key) @@ -1646,7 +1622,7 @@ func (s *store) CreateImage(id string, names []string, layer, metadata string, i if iOptions.Metadata != "" { options.Metadata = iOptions.Metadata } - options.BigData = append(options.BigData, copyImageBigDataOptionSlice(iOptions.BigData)...) + options.BigData = append(options.BigData, slices.Clone(iOptions.BigData)...) options.NamesHistory = append(options.NamesHistory, iOptions.NamesHistory...) if options.Flags == nil { options.Flags = make(map[string]interface{}) @@ -1760,8 +1736,8 @@ func (s *store) imageTopLayerForMapping(image *Image, ristore roImageStore, rlst layerOptions.IDMappingOptions = types.IDMappingOptions{ HostUIDMapping: options.HostUIDMapping, HostGIDMapping: options.HostGIDMapping, - UIDMap: copyIDMap(options.UIDMap), - GIDMap: copyIDMap(options.GIDMap), + UIDMap: copySlicePreferringNil(options.UIDMap), + GIDMap: copySlicePreferringNil(options.GIDMap), } } layerOptions.TemplateLayer = layer.ID @@ -1783,13 +1759,13 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat var options ContainerOptions if cOptions != nil { options = *cOptions - options.IDMappingOptions.UIDMap = copyIDMap(cOptions.IDMappingOptions.UIDMap) - options.IDMappingOptions.GIDMap = copyIDMap(cOptions.IDMappingOptions.GIDMap) - options.LabelOpts = copyStringSlice(cOptions.LabelOpts) + options.IDMappingOptions.UIDMap = copySlicePreferringNil(cOptions.IDMappingOptions.UIDMap) + options.IDMappingOptions.GIDMap = copySlicePreferringNil(cOptions.IDMappingOptions.GIDMap) + options.LabelOpts = copySlicePreferringNil(cOptions.LabelOpts) options.Flags = maps.Clone(cOptions.Flags) - options.MountOpts = copyStringSlice(cOptions.MountOpts) + options.MountOpts = copySlicePreferringNil(cOptions.MountOpts) options.StorageOpt = copyStringStringMap(cOptions.StorageOpt) - options.BigData = copyContainerBigDataOptionSlice(cOptions.BigData) + options.BigData = slices.Clone(cOptions.BigData) } if options.HostUIDMapping { options.UIDMap = nil @@ -1913,8 +1889,8 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat layerOptions.IDMappingOptions = types.IDMappingOptions{ HostUIDMapping: idMappingsOptions.HostUIDMapping, HostGIDMapping: idMappingsOptions.HostGIDMapping, - UIDMap: copyIDMap(uidMap), - GIDMap: copyIDMap(gidMap), + UIDMap: copySlicePreferringNil(uidMap), + GIDMap: copySlicePreferringNil(gidMap), } } if options.Flags == nil { @@ -1951,8 +1927,8 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat options.IDMappingOptions = types.IDMappingOptions{ HostUIDMapping: len(options.UIDMap) == 0, HostGIDMapping: len(options.GIDMap) == 0, - UIDMap: copyIDMap(options.UIDMap), - GIDMap: copyIDMap(options.GIDMap), + UIDMap: copySlicePreferringNil(options.UIDMap), + GIDMap: copySlicePreferringNil(options.GIDMap), } container, err := s.containerStore.create(id, names, imageID, layer, &options) if err != nil || container == nil { @@ -2450,9 +2426,9 @@ func (s *store) updateNames(id string, names []string, op updateNameOperation) e options := ImageOptions{ CreationDate: i.Created, Digest: i.Digest, - Digests: copyDigestSlice(i.Digests), + Digests: copySlicePreferringNil(i.Digests), Metadata: i.Metadata, - NamesHistory: copyStringSlice(i.NamesHistory), + NamesHistory: copySlicePreferringNil(i.NamesHistory), Flags: copyStringInterfaceMap(i.Flags), } for _, key := range i.BigDataNames { @@ -3674,23 +3650,18 @@ func makeBigDataBaseName(key string) string { } func stringSliceWithoutValue(slice []string, value string) []string { - modified := make([]string, 0, len(slice)) - for _, v := range slice { - if v == value { - continue - } - modified = append(modified, v) - } - return modified + return slices.DeleteFunc(slices.Clone(slice), func(v string) bool { + return v == value + }) } -func copyStringSlice(slice []string) []string { - if len(slice) == 0 { +// copySlicePreferringNil returns a copy of the slice. +// If s is empty, a nil is returned. +func copySlicePreferringNil[S ~[]E, E any](s S) S { + if len(s) == 0 { return nil } - ret := make([]string, len(slice)) - copy(ret, slice) - return ret + return slices.Clone(s) } func copyStringStringMap(m map[string]string) map[string]string { @@ -3701,15 +3672,6 @@ func copyStringStringMap(m map[string]string) map[string]string { return ret } -func copyDigestSlice(slice []digest.Digest) []digest.Digest { - if len(slice) == 0 { - return nil - } - ret := make([]digest.Digest, len(slice)) - copy(ret, slice) - return ret -} - // copyStringInterfaceMap still forces us to assume that the interface{} is // a non-pointer scalar value func copyStringInterfaceMap(m map[string]interface{}) map[string]interface{} { @@ -3720,31 +3682,6 @@ func copyStringInterfaceMap(m map[string]interface{}) map[string]interface{} { return ret } -func copyLayerBigDataOptionSlice(slice []LayerBigDataOption) []LayerBigDataOption { - ret := make([]LayerBigDataOption, len(slice)) - copy(ret, slice) - return ret -} - -func copyImageBigDataOptionSlice(slice []ImageBigDataOption) []ImageBigDataOption { - ret := make([]ImageBigDataOption, len(slice)) - for i := range slice { - ret[i].Key = slice[i].Key - ret[i].Data = slices.Clone(slice[i].Data) - ret[i].Digest = slice[i].Digest - } - return ret -} - -func copyContainerBigDataOptionSlice(slice []ContainerBigDataOption) []ContainerBigDataOption { - ret := make([]ContainerBigDataOption, len(slice)) - for i := range slice { - ret[i].Key = slice[i].Key - ret[i].Data = slices.Clone(slice[i].Data) - } - return ret -} - // AutoUserNsMinSize is the minimum size for automatically created user namespaces const AutoUserNsMinSize = 1024 From 13a96fe9dae462aea7fabb2fae7947d93dc7bb7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Rod=C3=A1k?= Date: Wed, 9 Oct 2024 10:47:44 +0200 Subject: [PATCH 2/2] Refactor of copying maps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes the duplicate copy*Map function using the general function newMapFrom. Reduces the allocation of empty maps using the copyMapPrefferingNil function. This change may affect the behavior so that instead of an empty allocated map, a nil will be returned. Signed-off-by: Jan Rodák --- containers.go | 9 ++++----- images.go | 9 ++++----- layers.go | 4 ++-- store.go | 30 +++++++++++++++++------------- 4 files changed, 27 insertions(+), 25 deletions(-) diff --git a/containers.go b/containers.go index 661fa6e314..143fde2971 100644 --- a/containers.go +++ b/containers.go @@ -3,7 +3,6 @@ package storage import ( "errors" "fmt" - "maps" "os" "path/filepath" "slices" @@ -169,12 +168,12 @@ func copyContainer(c *Container) *Container { LayerID: c.LayerID, Metadata: c.Metadata, BigDataNames: copySlicePreferringNil(c.BigDataNames), - BigDataSizes: maps.Clone(c.BigDataSizes), - BigDataDigests: maps.Clone(c.BigDataDigests), + BigDataSizes: copyMapPreferringNil(c.BigDataSizes), + BigDataDigests: copyMapPreferringNil(c.BigDataDigests), Created: c.Created, UIDMap: copySlicePreferringNil(c.UIDMap), GIDMap: copySlicePreferringNil(c.GIDMap), - Flags: maps.Clone(c.Flags), + Flags: copyMapPreferringNil(c.Flags), volatileStore: c.volatileStore, } } @@ -692,7 +691,7 @@ func (r *containerStore) create(id string, names []string, image, layer string, BigDataSizes: make(map[string]int64), BigDataDigests: make(map[string]digest.Digest), Created: time.Now().UTC(), - Flags: copyStringInterfaceMap(options.Flags), + Flags: newMapFrom(options.Flags), UIDMap: copySlicePreferringNil(options.UIDMap), GIDMap: copySlicePreferringNil(options.GIDMap), volatileStore: options.Volatile, diff --git a/images.go b/images.go index 7ea5b5fb6e..5c9127eded 100644 --- a/images.go +++ b/images.go @@ -2,7 +2,6 @@ package storage import ( "fmt" - "maps" "os" "path/filepath" "slices" @@ -190,11 +189,11 @@ func copyImage(i *Image) *Image { MappedTopLayers: copySlicePreferringNil(i.MappedTopLayers), Metadata: i.Metadata, BigDataNames: copySlicePreferringNil(i.BigDataNames), - BigDataSizes: maps.Clone(i.BigDataSizes), - BigDataDigests: maps.Clone(i.BigDataDigests), + BigDataSizes: copyMapPreferringNil(i.BigDataSizes), + BigDataDigests: copyMapPreferringNil(i.BigDataDigests), Created: i.Created, ReadOnly: i.ReadOnly, - Flags: maps.Clone(i.Flags), + Flags: copyMapPreferringNil(i.Flags), } } @@ -725,7 +724,7 @@ func (r *imageStore) create(id string, names []string, layer string, options Ima BigDataSizes: make(map[string]int64), BigDataDigests: make(map[string]digest.Digest), Created: options.CreationDate, - Flags: copyStringInterfaceMap(options.Flags), + Flags: newMapFrom(options.Flags), } if image.Created.IsZero() { image.Created = time.Now().UTC() diff --git a/layers.go b/layers.go index 504ac08f6d..b62150a2e6 100644 --- a/layers.go +++ b/layers.go @@ -452,7 +452,7 @@ func copyLayer(l *Layer) *Layer { ReadOnly: l.ReadOnly, volatileStore: l.volatileStore, BigDataNames: copySlicePreferringNil(l.BigDataNames), - Flags: maps.Clone(l.Flags), + Flags: copyMapPreferringNil(l.Flags), UIDMap: copySlicePreferringNil(l.UIDMap), GIDMap: copySlicePreferringNil(l.GIDMap), UIDs: copySlicePreferringNil(l.UIDs), @@ -1403,7 +1403,7 @@ func (r *layerStore) create(id string, parentLayer *Layer, names []string, mount CompressionType: templateCompressionType, UIDs: templateUIDs, GIDs: templateGIDs, - Flags: copyStringInterfaceMap(moreOptions.Flags), + Flags: newMapFrom(moreOptions.Flags), UIDMap: copySlicePreferringNil(moreOptions.UIDMap), GIDMap: copySlicePreferringNil(moreOptions.GIDMap), BigDataNames: []string{}, diff --git a/store.go b/store.go index 441f070509..347b9573bb 100644 --- a/store.go +++ b/store.go @@ -1446,7 +1446,7 @@ func (s *store) putLayer(rlstore rwLayerStore, rlstores []roLayerStore, id, pare if lOptions != nil { options = *lOptions options.BigData = slices.Clone(lOptions.BigData) - options.Flags = maps.Clone(lOptions.Flags) + options.Flags = copyMapPreferringNil(lOptions.Flags) } if options.HostUIDMapping { options.UIDMap = nil @@ -1762,9 +1762,9 @@ func (s *store) CreateContainer(id string, names []string, image, layer, metadat options.IDMappingOptions.UIDMap = copySlicePreferringNil(cOptions.IDMappingOptions.UIDMap) options.IDMappingOptions.GIDMap = copySlicePreferringNil(cOptions.IDMappingOptions.GIDMap) options.LabelOpts = copySlicePreferringNil(cOptions.LabelOpts) - options.Flags = maps.Clone(cOptions.Flags) + options.Flags = copyMapPreferringNil(cOptions.Flags) options.MountOpts = copySlicePreferringNil(cOptions.MountOpts) - options.StorageOpt = copyStringStringMap(cOptions.StorageOpt) + options.StorageOpt = copyMapPreferringNil(cOptions.StorageOpt) options.BigData = slices.Clone(cOptions.BigData) } if options.HostUIDMapping { @@ -2429,7 +2429,7 @@ func (s *store) updateNames(id string, names []string, op updateNameOperation) e Digests: copySlicePreferringNil(i.Digests), Metadata: i.Metadata, NamesHistory: copySlicePreferringNil(i.NamesHistory), - Flags: copyStringInterfaceMap(i.Flags), + Flags: copyMapPreferringNil(i.Flags), } for _, key := range i.BigDataNames { data, err := store.BigData(id, key) @@ -3664,18 +3664,22 @@ func copySlicePreferringNil[S ~[]E, E any](s S) S { return slices.Clone(s) } -func copyStringStringMap(m map[string]string) map[string]string { - ret := make(map[string]string, len(m)) - for k, v := range m { - ret[k] = v +// copyMapPreferringNil returns a shallow clone of map m. +// If m is empty, a nil is returned. +// +// (As of, e.g., Go 1.23, maps.Clone preserves nil, but that’s not a documented promise; +// and this function turns even non-nil empty maps into nil.) +func copyMapPreferringNil[K comparable, V any](m map[K]V) map[K]V { + if len(m) == 0 { + return nil } - return ret + return maps.Clone(m) } -// copyStringInterfaceMap still forces us to assume that the interface{} is -// a non-pointer scalar value -func copyStringInterfaceMap(m map[string]interface{}) map[string]interface{} { - ret := make(map[string]interface{}, len(m)) +// newMapFrom returns a shallow clone of map m. +// If m is empty, an empty map is allocated and returned. +func newMapFrom[K comparable, V any](m map[K]V) map[K]V { + ret := make(map[K]V, len(m)) for k, v := range m { ret[k] = v }