From c0f1a64adcbfb4190a8f02c9e819895d1c301df5 Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Thu, 3 Oct 2019 12:25:17 -0700 Subject: [PATCH 1/2] Use PathElementSet rather than map[PathElement.String()] --- fieldpath/element.go | 6 ++++++ typed/validate.go | 9 ++++----- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/fieldpath/element.go b/fieldpath/element.go index 9783c3ba..ed2ca27f 100644 --- a/fieldpath/element.go +++ b/fieldpath/element.go @@ -143,6 +143,12 @@ type PathElementSet struct { members sortedPathElements } +func MakePathElementSet(size int) PathElementSet { + return PathElementSet{ + members: make(sortedPathElements, 0, size), + } +} + type sortedPathElements []PathElement // Implement the sort interface; this would permit bulk creation, which would diff --git a/typed/validate.go b/typed/validate.go index cefa2a38..338c0583 100644 --- a/typed/validate.go +++ b/typed/validate.go @@ -142,7 +142,7 @@ func (v *validatingObjectWalker) doScalar(t *schema.Scalar) ValidationErrors { } func (v *validatingObjectWalker) visitListItems(t *schema.List, list *value.List) (errs ValidationErrors) { - observedKeys := map[string]struct{}{} + observedKeys := fieldpath.MakePathElementSet(len(list.Items)) for i, child := range list.Items { pe, err := listItemToPathElement(t, i, child) if err != nil { @@ -152,11 +152,10 @@ func (v *validatingObjectWalker) visitListItems(t *schema.List, list *value.List // this element. continue } - keyStr := pe.String() - if _, found := observedKeys[keyStr]; found { - errs = append(errs, v.errorf("duplicate entries for key %v", keyStr)...) + if observedKeys.Has(pe) { + errs = append(errs, v.errorf("duplicate entries for key %v", pe.String())...) } - observedKeys[keyStr] = struct{}{} + observedKeys.Insert(pe) v2 := v.prepareDescent(pe, t.ElementType) v2.value = child errs = append(errs, v2.validate()...) From 5d9b53d9d88701e7e67ef0a6547d123288de1dd9 Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Thu, 3 Oct 2019 12:51:32 -0700 Subject: [PATCH 2/2] Create map of PathElement to Value and use it instead of Strings --- fieldpath/pathelementmap.go | 85 ++++++++++++++++++++++++++++++++ fieldpath/pathelementmap_test.go | 50 +++++++++++++++++++ typed/merge.go | 46 ++++++++--------- 3 files changed, 158 insertions(+), 23 deletions(-) create mode 100644 fieldpath/pathelementmap.go create mode 100644 fieldpath/pathelementmap_test.go diff --git a/fieldpath/pathelementmap.go b/fieldpath/pathelementmap.go new file mode 100644 index 00000000..f35089a1 --- /dev/null +++ b/fieldpath/pathelementmap.go @@ -0,0 +1,85 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package fieldpath + +import ( + "sort" + + "sigs.k8s.io/structured-merge-diff/value" +) + +// PathElementValueMap is a map from PathElement to value.Value. +// +// TODO(apelisse): We have multiple very similar implementation of this +// for PathElementSet and SetNodeMap, so we could probably share the +// code. +type PathElementValueMap struct { + members sortedPathElementValues +} + +func MakePathElementValueMap(size int) PathElementValueMap { + return PathElementValueMap{ + members: make(sortedPathElementValues, 0, size), + } +} + +type pathElementValue struct { + PathElement PathElement + Value value.Value +} + +type sortedPathElementValues []pathElementValue + +// Implement the sort interface; this would permit bulk creation, which would +// be faster than doing it one at a time via Insert. +func (spev sortedPathElementValues) Len() int { return len(spev) } +func (spev sortedPathElementValues) Less(i, j int) bool { + return spev[i].PathElement.Less(spev[j].PathElement) +} +func (spev sortedPathElementValues) Swap(i, j int) { spev[i], spev[j] = spev[j], spev[i] } + +// Insert adds the pathelement and associated value in the map. +func (s *PathElementValueMap) Insert(pe PathElement, v value.Value) { + loc := sort.Search(len(s.members), func(i int) bool { + return !s.members[i].PathElement.Less(pe) + }) + if loc == len(s.members) { + s.members = append(s.members, pathElementValue{pe, v}) + return + } + if s.members[loc].PathElement.Equals(pe) { + return + } + s.members = append(s.members, pathElementValue{}) + copy(s.members[loc+1:], s.members[loc:]) + s.members[loc] = pathElementValue{pe, v} +} + +// Get retrieves the value associated with the given PathElement from the map. +// (nil, false) is returned if there is no such PathElement. +func (s *PathElementValueMap) Get(pe PathElement) (value.Value, bool) { + loc := sort.Search(len(s.members), func(i int) bool { + return !s.members[i].PathElement.Less(pe) + }) + if loc == len(s.members) { + return value.Value{}, false + } + if s.members[loc].PathElement.Equals(pe) { + return s.members[loc].Value, true + } + return value.Value{}, false +} diff --git a/fieldpath/pathelementmap_test.go b/fieldpath/pathelementmap_test.go new file mode 100644 index 00000000..51fa72f3 --- /dev/null +++ b/fieldpath/pathelementmap_test.go @@ -0,0 +1,50 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package fieldpath + +import ( + "testing" + + "sigs.k8s.io/structured-merge-diff/value" +) + +func TestPathElementValueMap(t *testing.T) { + m := PathElementValueMap{} + + if _, ok := m.Get(PathElement{FieldName: strptr("onion")}); ok { + t.Fatal("Unexpected path-element found in empty map") + } + + m.Insert(PathElement{FieldName: strptr("carrot")}, value.StringValue("knife")) + m.Insert(PathElement{FieldName: strptr("chive")}, value.IntValue(2)) + + if _, ok := m.Get(PathElement{FieldName: strptr("onion")}); ok { + t.Fatal("Unexpected path-element in map") + } + + if val, ok := m.Get(PathElement{FieldName: strptr("carrot")}); !ok { + t.Fatal("Missing path-element in map") + } else if !val.Equals(value.StringValue("knife")) { + t.Fatalf("Unexpected value found: %#v", val) + } + + if val, ok := m.Get(PathElement{FieldName: strptr("chive")}); !ok { + t.Fatal("Missing path-element in map") + } else if !val.Equals(value.IntValue(2)) { + t.Fatalf("Unexpected value found: %#v", val) + } +} diff --git a/typed/merge.go b/typed/merge.go index 0753e716..bb92f84b 100644 --- a/typed/merge.go +++ b/typed/merge.go @@ -168,8 +168,9 @@ func (w *mergingWalker) visitListItems(t *schema.List, lhs, rhs *value.List) (er rhsOrder := []fieldpath.PathElement{} // First, collect all RHS children. - observedRHS := map[string]value.Value{} + var observedRHS fieldpath.PathElementValueMap if rhs != nil { + observedRHS = fieldpath.MakePathElementValueMap(len(rhs.Items)) for i, child := range rhs.Items { pe, err := listItemToPathElement(t, i, child) if err != nil { @@ -179,18 +180,18 @@ func (w *mergingWalker) visitListItems(t *schema.List, lhs, rhs *value.List) (er // this element. continue } - keyStr := pe.String() - if _, found := observedRHS[keyStr]; found { - errs = append(errs, w.errorf("rhs: duplicate entries for key %v", keyStr)...) + if _, ok := observedRHS.Get(pe); ok { + errs = append(errs, w.errorf("rhs: duplicate entries for key %v", pe.String())...) } - observedRHS[keyStr] = child + observedRHS.Insert(pe, child) rhsOrder = append(rhsOrder, pe) } } // Then merge with LHS children. - observedLHS := map[string]struct{}{} + var observedLHS fieldpath.PathElementSet if lhs != nil { + observedLHS = fieldpath.MakePathElementSet(len(lhs.Items)) for i, child := range lhs.Items { pe, err := listItemToPathElement(t, i, child) if err != nil { @@ -200,15 +201,14 @@ func (w *mergingWalker) visitListItems(t *schema.List, lhs, rhs *value.List) (er // this element. continue } - keyStr := pe.String() - if _, found := observedLHS[keyStr]; found { - errs = append(errs, w.errorf("lhs: duplicate entries for key %v", keyStr)...) + if observedLHS.Has(pe) { + errs = append(errs, w.errorf("lhs: duplicate entries for key %v", pe.String())...) continue } - observedLHS[keyStr] = struct{}{} + observedLHS.Insert(pe) w2 := w.prepareDescent(pe, t.ElementType) w2.lhs = &child - if rchild, ok := observedRHS[keyStr]; ok { + if rchild, ok := observedRHS.Get(pe); ok { w2.rhs = &rchild } if newErrs := w2.merge(); len(newErrs) > 0 { @@ -217,22 +217,22 @@ func (w *mergingWalker) visitListItems(t *schema.List, lhs, rhs *value.List) (er out.Items = append(out.Items, *w2.out) } w.finishDescent(w2) - // Keep track of children that have been handled - delete(observedRHS, keyStr) } } - for _, rhsToCheck := range rhsOrder { - if unmergedChild, ok := observedRHS[rhsToCheck.String()]; ok { - w2 := w.prepareDescent(rhsToCheck, t.ElementType) - w2.rhs = &unmergedChild - if newErrs := w2.merge(); len(newErrs) > 0 { - errs = append(errs, newErrs...) - } else if w2.out != nil { - out.Items = append(out.Items, *w2.out) - } - w.finishDescent(w2) + for _, pe := range rhsOrder { + if observedLHS.Has(pe) { + continue + } + value, _ := observedRHS.Get(pe) + w2 := w.prepareDescent(pe, t.ElementType) + w2.rhs = &value + if newErrs := w2.merge(); len(newErrs) > 0 { + errs = append(errs, newErrs...) + } else if w2.out != nil { + out.Items = append(out.Items, *w2.out) } + w.finishDescent(w2) } if len(out.Items) > 0 {