From 2fda12d22033fa1d04d7060e73d73e97da2dd98a Mon Sep 17 00:00:00 2001 From: Carl Henrik Lunde Date: Thu, 2 Nov 2023 22:54:42 +0100 Subject: [PATCH] perf: MakeDefaultConfig once (#5082) * perf: MakeDefaultConfig once This shaves of another 2 seconds (62%) of the remaining execution time for a kustomization tree with 4000 documents, reducing the execution time from 4.79s to 1.82s 0 0% 1.38% 2.98s 37.25% sigs.k8s.io/kustomize/api/internal/plugins/builtinconfig.MakeDefaultConfig before: (pprof) top30 -cum Showing nodes accounting for 1.82s, 22.75% of 8s total Dropped 408 nodes (cum <= 0.04s) Showing top 30 nodes out of 308 flat flat% sum% cum cum% 0 0% 0% 4.79s 59.88% github.com/spf13/cobra.(*Command).Execute 0 0% 0% 4.79s 59.88% github.com/spf13/cobra.(*Command).ExecuteC 0 0% 0% 4.79s 59.88% github.com/spf13/cobra.(*Command).execute 0 0% 0% 4.79s 59.88% main.main 0 0% 0% 4.79s 59.88% runtime.main 0 0% 0% 4.79s 59.88% sigs.k8s.io/kustomize/kustomize/v5/commands/build.NewCmdBuild.func1 0 0% 0% 4.22s 52.75% sigs.k8s.io/kustomize/api/krusty.(*Kustomizer).Run 0 0% 0% 4.18s 52.25% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).MakeCustomizedResMap (inline) 0 0% 0% 4.18s 52.25% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).makeCustomizedResMap 0 0% 0% 4.06s 50.75% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).AccumulateTarget 0 0% 0% 4.06s 50.75% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).accumulateResources 0 0% 0% 4.06s 50.75% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).accumulateTarget 0 0% 0% 4.05s 50.62% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).accumulateDirectory 0 0% 0% 3.22s 40.25% runtime.systemstack 0 0% 0% 3.03s 37.88% runtime.gcBgMarkWorker 0 0% 0% 3.03s 37.88% runtime.gcBgMarkWorker.func2 0.11s 1.38% 1.38% 3.03s 37.88% runtime.gcDrain 0 0% 1.38% 2.98s 37.25% sigs.k8s.io/kustomize/api/internal/plugins/builtinconfig.MakeDefaultConfig 0 0% 1.38% 2.98s 37.25% sigs.k8s.io/kustomize/api/internal/plugins/builtinconfig.MakeTransformerConfig 0 0% 1.38% 2.98s 37.25% sigs.k8s.io/kustomize/api/internal/plugins/builtinconfig.makeTransformerConfigFromBytes 0 0% 1.38% 2.97s 37.12% sigs.k8s.io/yaml.yamlUnmarshal 0 0% 1.38% 2.91s 36.38% sigs.k8s.io/yaml.Unmarshal (inline) 1.34s 16.75% 18.12% 2.87s 35.88% runtime.scanobject 0 0% 18.12% 2.53s 31.62% sigs.k8s.io/yaml.yamlToJSON 0 0% 18.12% 1.91s 23.88% gopkg.in/yaml%2ev2.unmarshal 0 0% 18.12% 1.86s 23.25% gopkg.in/yaml%2ev2.Unmarshal (partial-inline) 0 0% 18.12% 1.43s 17.88% gopkg.in/yaml%2ev2.(*parser).parse 0.01s 0.12% 18.25% 1.27s 15.88% gopkg.in/yaml%2ev2.(*parser).document 0.01s 0.12% 18.38% 1.25s 15.62% gopkg.in/yaml%2ev2.(*parser).mapping 0.35s 4.38% 22.75% 1.21s 15.12% runtime.mallocgc after: (pprof) top30 -cum Showing nodes accounting for 0.84s, 24.42% of 3.44s total Dropped 225 nodes (cum <= 0.02s) Showing top 30 nodes out of 345 flat flat% sum% cum cum% 0 0% 0% 1.82s 52.91% github.com/spf13/cobra.(*Command).Execute 0 0% 0% 1.82s 52.91% github.com/spf13/cobra.(*Command).ExecuteC 0 0% 0% 1.82s 52.91% github.com/spf13/cobra.(*Command).execute 0 0% 0% 1.82s 52.91% main.main 0 0% 0% 1.82s 52.91% runtime.main 0 0% 0% 1.82s 52.91% sigs.k8s.io/kustomize/kustomize/v5/commands/build.NewCmdBuild.func1 0 0% 0% 1.57s 45.64% runtime.systemstack 0 0% 0% 1.49s 43.31% runtime.gcBgMarkWorker 0 0% 0% 1.49s 43.31% runtime.gcBgMarkWorker.func2 0.03s 0.87% 0.87% 1.49s 43.31% runtime.gcDrain 0.62s 18.02% 18.90% 1.45s 42.15% runtime.scanobject 0 0% 18.90% 1.25s 36.34% sigs.k8s.io/kustomize/api/krusty.(*Kustomizer).Run 0 0% 18.90% 1.21s 35.17% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).MakeCustomizedResMap (inline) 0 0% 18.90% 1.21s 35.17% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).makeCustomizedResMap 0 0% 18.90% 1.08s 31.40% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).AccumulateTarget 0 0% 18.90% 1.08s 31.40% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).accumulateResources 0 0% 18.90% 1.08s 31.40% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).accumulateTarget 0 0% 18.90% 1.07s 31.10% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).accumulateDirectory 0 0% 18.90% 0.57s 16.57% sigs.k8s.io/kustomize/api/resmap.(*resWrangler).AsYaml 0 0% 18.90% 0.57s 16.57% sigs.k8s.io/kustomize/api/resource.(*Resource).AsYAML 0.11s 3.20% 22.09% 0.48s 13.95% runtime.mallocgc 0 0% 22.09% 0.45s 13.08% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).accumulateFile 0 0% 22.09% 0.45s 13.08% sigs.k8s.io/kustomize/api/resmap.(*Factory).FromFile 0 0% 22.09% 0.33s 9.59% sigs.k8s.io/kustomize/kyaml/yaml.(*RNode).MarshalJSON 0.08s 2.33% 24.42% 0.32s 9.30% runtime.greyobject 0 0% 24.42% 0.30s 8.72% sigs.k8s.io/kustomize/api/resmap.(*Factory).NewResMapFromBytes 0 0% 24.42% 0.27s 7.85% sigs.k8s.io/yaml.JSONToYAML 0 0% 24.42% 0.25s 7.27% sigs.k8s.io/kustomize/api/internal/target.(*KustTarget).runTransformers 0 0% 24.42% 0.25s 7.27% sigs.k8s.io/kustomize/api/resource.(*Factory).RNodesFromBytes 0 0% 24.42% 0.25s 7.27% sigs.k8s.io/kustomize/api/resource.(*Factory).SliceFromBytes * Tests and comments for MakeDefaultConfig perf work Document updated code in MakeDefaultConfig. Add unit tests to ensure DeepCopy works. Add hints to other code to ensure DeepCopy is kept up to date. --- .../builtinconfig/namebackreferences.go | 13 ++++++ .../builtinconfig/namebackreferences_test.go | 26 +++++++++++ .../builtinconfig/transformerconfig.go | 43 ++++++++++++++++--- .../builtinconfig/transformerconfig_test.go | 19 ++++++++ api/types/fieldspec.go | 9 ++++ api/types/fieldspec_test.go | 24 +++++++++++ 6 files changed, 128 insertions(+), 6 deletions(-) diff --git a/api/internal/plugins/builtinconfig/namebackreferences.go b/api/internal/plugins/builtinconfig/namebackreferences.go index 36ef42c27..eb0f30733 100644 --- a/api/internal/plugins/builtinconfig/namebackreferences.go +++ b/api/internal/plugins/builtinconfig/namebackreferences.go @@ -47,6 +47,8 @@ type NameBackReferences struct { // TODO: rename json 'fieldSpecs' to 'referrers' for clarity. // This will, however, break anyone using a custom config. Referrers types.FsSlice `json:"fieldSpecs,omitempty" yaml:"fieldSpecs,omitempty"` + + // Note: If any new pointer based members are added, DeepCopy needs to be updated } func (n NameBackReferences) String() string { @@ -66,6 +68,17 @@ func (s nbrSlice) Less(i, j int) bool { return s[i].Gvk.IsLessThan(s[j].Gvk) } +// DeepCopy returns a new copy of nbrSlice +func (s nbrSlice) DeepCopy() nbrSlice { + ret := make(nbrSlice, len(s)) + copy(ret, s) + for i, slice := range ret { + ret[i].Referrers = slice.Referrers.DeepCopy() + } + + return ret +} + func (s nbrSlice) mergeAll(o nbrSlice) (result nbrSlice, err error) { result = s for _, r := range o { diff --git a/api/internal/plugins/builtinconfig/namebackreferences_test.go b/api/internal/plugins/builtinconfig/namebackreferences_test.go index b9b60369b..0e51b80cf 100644 --- a/api/internal/plugins/builtinconfig/namebackreferences_test.go +++ b/api/internal/plugins/builtinconfig/namebackreferences_test.go @@ -95,3 +95,29 @@ func TestMergeAll(t *testing.T) { t.Fatalf("expected\n %v\n but got\n %v\n", expected, actual) } } + +func TestNbrSlice_DeepCopy(t *testing.T) { + original := make(nbrSlice, 2, 4) + original[0] = NameBackReferences{Gvk: resid.FromKind("A"), Referrers: types.FsSlice{{Path: "a"}}} + original[1] = NameBackReferences{Gvk: resid.FromKind("B"), Referrers: types.FsSlice{{Path: "b"}}} + + copied := original.DeepCopy() + + original, _ = original.mergeOne(NameBackReferences{Gvk: resid.FromKind("C"), Referrers: types.FsSlice{{Path: "c"}}}) + + // perform mutations which should not affect original + copied.Swap(0, 1) + copied[0].Referrers[0].Path = "very b" // ensure Referrers are not shared + _, _ = copied.mergeOne(NameBackReferences{Gvk: resid.FromKind("D"), Referrers: types.FsSlice{{Path: "d"}}}) + + // if DeepCopy does not work, original would be {very b,a,d} instead of {a,b,c} + expected := nbrSlice{ + {Gvk: resid.FromKind("A"), Referrers: types.FsSlice{{Path: "a"}}}, + {Gvk: resid.FromKind("B"), Referrers: types.FsSlice{{Path: "b"}}}, + {Gvk: resid.FromKind("C"), Referrers: types.FsSlice{{Path: "c"}}}, + } + + if !reflect.DeepEqual(original, expected) { + t.Fatalf("original affected by mutations to copied object:\ngot\t%+v,\nexpected: %+v", original, expected) + } +} diff --git a/api/internal/plugins/builtinconfig/transformerconfig.go b/api/internal/plugins/builtinconfig/transformerconfig.go index 8b714d815..ab847eb6e 100644 --- a/api/internal/plugins/builtinconfig/transformerconfig.go +++ b/api/internal/plugins/builtinconfig/transformerconfig.go @@ -6,6 +6,7 @@ package builtinconfig import ( "log" "sort" + "sync" "sigs.k8s.io/kustomize/api/ifc" "sigs.k8s.io/kustomize/api/internal/konfig/builtinpluginconsts" @@ -15,6 +16,7 @@ import ( // TransformerConfig holds the data needed to perform transformations. type TransformerConfig struct { + // if any fields are added, update the DeepCopy implementation NamePrefix types.FsSlice `json:"namePrefix,omitempty" yaml:"namePrefix,omitempty"` NameSuffix types.FsSlice `json:"nameSuffix,omitempty" yaml:"nameSuffix,omitempty"` NameSpace types.FsSlice `json:"namespace,omitempty" yaml:"namespace,omitempty"` @@ -32,14 +34,43 @@ func MakeEmptyConfig() *TransformerConfig { return &TransformerConfig{} } +// DeepCopy returns a new copy of TransformerConfig +func (t *TransformerConfig) DeepCopy() *TransformerConfig { + return &TransformerConfig{ + NamePrefix: t.NamePrefix.DeepCopy(), + NameSuffix: t.NameSuffix.DeepCopy(), + NameSpace: t.NameSpace.DeepCopy(), + CommonLabels: t.CommonLabels.DeepCopy(), + TemplateLabels: t.TemplateLabels.DeepCopy(), + CommonAnnotations: t.CommonAnnotations.DeepCopy(), + NameReference: t.NameReference.DeepCopy(), + VarReference: t.VarReference.DeepCopy(), + Images: t.Images.DeepCopy(), + Replicas: t.Replicas.DeepCopy(), + } +} + +// the default transformer config is initialized by MakeDefaultConfig, +// and must only be accessed via that function. +var ( + initDefaultConfig sync.Once //nolint:gochecknoglobals + defaultConfig *TransformerConfig //nolint:gochecknoglobals +) + // MakeDefaultConfig returns a default TransformerConfig. func MakeDefaultConfig() *TransformerConfig { - c, err := makeTransformerConfigFromBytes( - builtinpluginconsts.GetDefaultFieldSpecs()) - if err != nil { - log.Fatalf("Unable to make default transformconfig: %v", err) - } - return c + // parsing is expensive when having a large tree with many kustomization modules, so only do it once + initDefaultConfig.Do(func() { + var err error + defaultConfig, err = makeTransformerConfigFromBytes( + builtinpluginconsts.GetDefaultFieldSpecs()) + if err != nil { + log.Fatalf("Unable to make default transformconfig: %v", err) + } + }) + + // return a copy to avoid any mutations to protect the reference copy + return defaultConfig.DeepCopy() } // MakeTransformerConfig returns a merger of custom config, diff --git a/api/internal/plugins/builtinconfig/transformerconfig_test.go b/api/internal/plugins/builtinconfig/transformerconfig_test.go index 112a2d8cd..e3b3bfe13 100644 --- a/api/internal/plugins/builtinconfig/transformerconfig_test.go +++ b/api/internal/plugins/builtinconfig/transformerconfig_test.go @@ -173,3 +173,22 @@ func TestMerge(t *testing.T) { t.Fatalf("expected: %v\n but got: %v\n", cfga, actual) } } + +func TestMakeDefaultConfig_mutation(t *testing.T) { + a := MakeDefaultConfig() + + // mutate + a.NameReference[0].Kind = "mutated" + a.NameReference = a.NameReference[:1] + + clean := MakeDefaultConfig() + if clean.NameReference[0].Kind == "mutated" { + t.Errorf("MakeDefaultConfig() did not return a clean copy: %+v", clean.NameReference) + } +} + +func BenchmarkMakeDefaultConfig(b *testing.B) { + for i := 0; i < b.N; i++ { + _ = MakeDefaultConfig() + } +} diff --git a/api/types/fieldspec.go b/api/types/fieldspec.go index 8d3579544..8b78889ea 100644 --- a/api/types/fieldspec.go +++ b/api/types/fieldspec.go @@ -30,6 +30,8 @@ type FieldSpec struct { resid.Gvk `json:",inline,omitempty" yaml:",inline,omitempty"` Path string `json:"path,omitempty" yaml:"path,omitempty"` CreateIfNotPresent bool `json:"create,omitempty" yaml:"create,omitempty"` + + // Note: If any new pointer based members are added, FsSlice.DeepCopy needs to be updated } func (fs FieldSpec) String() string { @@ -50,6 +52,13 @@ func (s FsSlice) Less(i, j int) bool { return s[i].Gvk.IsLessThan(s[j].Gvk) } +// DeepCopy returns a new copy of FsSlice +func (s FsSlice) DeepCopy() FsSlice { + ret := make(FsSlice, len(s)) + copy(ret, s) + return ret +} + // MergeAll merges the argument into this, returning the result. // Items already present are ignored. // Items that conflict (primary key matches, but remain data differs) diff --git a/api/types/fieldspec_test.go b/api/types/fieldspec_test.go index c0518b94a..83edd73f1 100644 --- a/api/types/fieldspec_test.go +++ b/api/types/fieldspec_test.go @@ -142,3 +142,27 @@ func TestFsSlice_MergeAll(t *testing.T) { } } } + +func TestFsSlice_DeepCopy(t *testing.T) { + original := make(FsSlice, 2, 4) + original[0] = FieldSpec{Path: "a"} + original[1] = FieldSpec{Path: "b"} + + copied := original.DeepCopy() + + original, _ = original.MergeOne(FieldSpec{Path: "c"}) + + // perform mutations which should not affect original + copied.Swap(0, 1) + _, _ = copied.MergeOne(FieldSpec{Path: "d"}) + + // if DeepCopy does not work, original would be {b,a,d} instead of {a,b,c} + expected := FsSlice{ + {Path: "a"}, + {Path: "b"}, + {Path: "c"}, + } + if !reflect.DeepEqual(original, expected) { + t.Fatalf("original affected by mutations to copied object:\ngot\t%+v,\nexpected: %+v", original, expected) + } +}