Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: Improve resmap performance with AppendAll and Transform functions #5427

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
14 changes: 6 additions & 8 deletions api/internal/accumulator/resaccumulator.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,15 +171,13 @@ func (ra *ResAccumulator) FixBackReferences() (err error) {
// Intersection drops the resources which "other" does not have.
func (ra *ResAccumulator) Intersection(other resmap.ResMap) error {
otherIds := other.AllIds()
seen := make(map[resid.ResId]struct{}, len(otherIds))
for _, id := range otherIds {
seen[id] = struct{}{}
}
for _, curId := range ra.resMap.AllIds() {
toDelete := true
for _, otherId := range otherIds {
if otherId == curId {
toDelete = false
break
}
}
if toDelete {
_, inOtherSet := seen[curId]
if !inOtherSet {
err := ra.resMap.Remove(curId)
if err != nil {
return err
Expand Down
21 changes: 7 additions & 14 deletions api/internal/builtins/NamespaceTransformer.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 4 additions & 5 deletions api/internal/builtins/SortOrderTransformer.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 2 additions & 7 deletions api/resmap/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,8 @@ func (rmF *Factory) FromSecretArgs(
func newResMapFromResourceSlice(
resources []*resource.Resource) (ResMap, error) {
result := New()
for _, res := range resources {
err := result.Append(res)
if err != nil {
return nil, err
}
}
return result, nil
err := result.AppendMany(resources...)
return result, err
}

// NewResMapFromRNodeSlice returns a ResMap from a slice of RNodes
Expand Down
4 changes: 4 additions & 0 deletions api/resmap/resmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ type ResMap interface {
// in the cluster with the same Id.
Append(*resource.Resource) error

AppendMany(res ...*resource.Resource) error

// AppendAll appends another ResMap to self,
// failing on any CurId collision.
AppendAll(ResMap) error
Expand Down Expand Up @@ -330,4 +332,6 @@ type ResMap interface {
// struct data members, i.e. the Resource struct is replaced by RNode
// and use of (slow) k8s metadata annotations inside the RNode.
ApplyFilter(f kio.Filter) error

Transform(func(r *resource.Resource) error) error
}
45 changes: 39 additions & 6 deletions api/resmap/reswrangler.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,27 @@ func (m *resWrangler) Append(res *resource.Resource) error {
return nil
}

// Append implements ResMap.
func (m *resWrangler) AppendMany(resources ...*resource.Resource) error {
ids := m.AllIds()
seen := make(map[resid.ResId]struct{}, len(ids)+len(resources))
for _, id := range ids {
seen[id] = struct{}{}
}

for _, res := range resources {
newId := res.CurId()
if _, ok := seen[newId]; ok {
return fmt.Errorf(
"may not add resource with an already registered id: %s", newId)
}
seen[newId] = struct{}{}
m.append(res)
}

return nil
}

// append appends without performing an Id check
func (m *resWrangler) append(res *resource.Resource) {
m.rList = append(m.rList, res)
Expand Down Expand Up @@ -464,12 +485,7 @@ func (m *resWrangler) AppendAll(other ResMap) error {

// appendAll appends all the resources, error on Id collision.
func (m *resWrangler) appendAll(list []*resource.Resource) error {
for _, res := range list {
if err := m.Append(res); err != nil {
return err
}
}
return nil
return m.AppendMany(list...)
}

// AbsorbAll implements ResMap.
Expand Down Expand Up @@ -519,6 +535,23 @@ func (m *resWrangler) RemoveOriginAnnotations() error {
return nil
}

// Transform implements ResMap
func (m *resWrangler) Transform(cb func(*resource.Resource) error) error {
ids := make(map[resid.ResId]*resource.Resource)
for _, res := range m.rList {
if err := cb(res); err != nil {
return err
}
newId := res.CurId()
if other, found := ids[newId]; found {
return fmt.Errorf(
"transformation produces ID conflict: %+v, %+v", res, other)
}
ids[newId] = res
}
return nil
}

// AddTransformerAnnotation implements ResMap
func (m *resWrangler) AddTransformerAnnotation(origin *resource.Origin) error {
for _, res := range m.rList {
Expand Down
22 changes: 7 additions & 15 deletions plugin/builtin/namespacetransformer/NamespaceTransformer.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,9 @@
package main

import (
"fmt"

"sigs.k8s.io/kustomize/api/filters/namespace"
"sigs.k8s.io/kustomize/api/resmap"
"sigs.k8s.io/kustomize/api/resource"
"sigs.k8s.io/kustomize/api/types"
"sigs.k8s.io/kustomize/kyaml/errors"
"sigs.k8s.io/yaml"
Expand Down Expand Up @@ -49,25 +48,18 @@
if len(p.Namespace) == 0 {
return nil
}
for _, r := range m.Resources() {

return m.Transform(func(r *resource.Resource) error {

Check failure on line 52 in plugin/builtin/namespacetransformer/NamespaceTransformer.go

View workflow job for this annotation

GitHub Actions / Lint

error returned from interface method should be wrapped: sig: func (sigs.k8s.io/kustomize/api/resmap.ResMap).Transform(func(r *sigs.k8s.io/kustomize/api/resource.Resource) error) error (wrapcheck)
if r.IsNilOrEmpty() {
// Don't mutate empty objects?
continue
return nil
}
r.StorePreviousId()
if err := r.ApplyFilter(namespace.Filter{
return r.ApplyFilter(namespace.Filter{

Check failure on line 58 in plugin/builtin/namespacetransformer/NamespaceTransformer.go

View workflow job for this annotation

GitHub Actions / Lint

error returned from external package is unwrapped: sig: func (*sigs.k8s.io/kustomize/api/resource.Resource).ApplyFilter(f sigs.k8s.io/kustomize/kyaml/kio.Filter) error (wrapcheck)
Namespace: p.Namespace,
FsSlice: p.FieldSpecs,
SetRoleBindingSubjects: p.SetRoleBindingSubjects,
UnsetOnly: p.UnsetOnly,
}); err != nil {
return err
}
matches := m.GetMatchingResourcesByCurrentId(r.CurId().Equals)
if len(matches) != 1 {
return fmt.Errorf(
"namespace transformation produces ID conflict: %+v", matches)
}
}
return nil
})
})
}
9 changes: 4 additions & 5 deletions plugin/builtin/sortordertransformer/SortOrderTransformer.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,9 @@ func (p *plugin) Transform(m resmap.ResMap) (err error) {

// Clear the map and re-add the resources in the sorted order.
m.Clear()
for _, r := range s.resources {
err := m.Append(r)
if err != nil {
return errors.WrapPrefixf(err, "SortOrderTransformer: Failed to append to resources")
}
err := m.AppendMany(s.resources...)
if err != nil {
return errors.WrapPrefixf(err, "SortOrderTransformer: Failed to append to resources")
}
}
return nil
Expand All @@ -102,6 +100,7 @@ func (p *plugin) Transform(m resmap.ResMap) (err error) {
type legacyIDSorter struct {
// resids only stores the metadata of the object. This is an optimization as
// it's expensive to compute these again and again during ordering.

resids []resid.ResId
// Initially, we sorted the metadata (ResId) of each object and then called GetByCurrentId on each to construct the final list.
// The problem is that GetByCurrentId is inefficient and does a linear scan in a list every time we do that.
Expand Down
Loading