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: MakeDefaultConfig once #5082

Merged
merged 2 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions api/internal/plugins/builtinconfig/namebackreferences.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
26 changes: 26 additions & 0 deletions api/internal/plugins/builtinconfig/namebackreferences_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
43 changes: 37 additions & 6 deletions api/internal/plugins/builtinconfig/transformerconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package builtinconfig
import (
"log"
"sort"
"sync"

"sigs.k8s.io/kustomize/api/ifc"
"sigs.k8s.io/kustomize/api/internal/konfig/builtinpluginconsts"
Expand All @@ -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"`
Expand All @@ -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(),
chlunde marked this conversation as resolved.
Show resolved Hide resolved
}
}

// 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()
chlunde marked this conversation as resolved.
Show resolved Hide resolved
}

// MakeTransformerConfig returns a merger of custom config,
Expand Down
19 changes: 19 additions & 0 deletions api/internal/plugins/builtinconfig/transformerconfig_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}
9 changes: 9 additions & 0 deletions api/types/fieldspec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
Expand Down
24 changes: 24 additions & 0 deletions api/types/fieldspec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Loading