deepcopy cached value instead of unmarshalling json (#1439)

Instead of keeping the assigned value for AssignMutator as json, cache the unmarshalled value.

Unmarshalling JSON is incredibly expensive, even for simple values. We should avoid this when possible. Instead, we should cache the value to assign, and then deepcopy it when needed. Fortunately, the runtime package provides us a method just for this!

This change results in a ~90% reduction in compute time for most common uses. The existing benchmarks are for assigning a string, but the speedup is even more for complex assigned values.

Signed-off-by: Will Beason <willbeason@google.com>

pkg/mutation/mutators/assign_mutator.go

@@ -18,6 +18,7 @@ import (
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
 	runtimeschema "k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
@@ -28,9 +29,11 @@ var log = logf.Log.WithName("mutation").WithValues(logging.Process, "mutation")
 // AssignMutator is a mutator object built out of a
 // Assign instance.
 type AssignMutator struct {
-	id     types.ID
-	assign *mutationsv1alpha1.Assign
-	path   parser.Path
+	id          types.ID
+	assign      *mutationsv1alpha1.Assign
+	assignValue interface{}
+
+	path parser.Path
 
 	// bindings are the set of GVKs this Mutator applies to.
 	bindings  []runtimeschema.GroupVersionKind
@@ -98,7 +101,7 @@ func (m *AssignMutator) SchemaBindings() []runtimeschema.GroupVersionKind {
 }
 
 func (m *AssignMutator) Value() (interface{}, error) {
-	return types.UnmarshalValue(m.assign.Spec.Parameters.Assign.Raw)
+	return runtime.DeepCopyJSONValue(m.assignValue), nil
 }
 
 func (m *AssignMutator) HasDiff(mutator types.Mutator) bool {
@@ -131,13 +134,15 @@ func (m *AssignMutator) Path() parser.Path {
 
 func (m *AssignMutator) DeepCopy() types.Mutator {
 	res := &AssignMutator{
-		id:     m.id,
-		assign: m.assign.DeepCopy(),
+		id:          m.id,
+		assign:      m.assign.DeepCopy(),
+		assignValue: runtime.DeepCopyJSONValue(m.assignValue),
 		path: parser.Path{
 			Nodes: make([]parser.Node, len(m.path.Nodes)),
 		},
 		bindings: make([]runtimeschema.GroupVersionKind, len(m.bindings)),
 	}
+
 	copy(res.path.Nodes, m.path.Nodes)
 	copy(res.bindings, m.bindings)
 	res.tester = m.tester.DeepCopy()
@@ -208,12 +213,13 @@ func MutatorForAssign(assign *mutationsv1alpha1.Assign) (*AssignMutator, error)
 	}
 
 	return &AssignMutator{
-		id:        id,
-		assign:    assign.DeepCopy(),
-		bindings:  gvks,
-		path:      path,
-		tester:    tester,
-		valueTest: &valueTests,
+		id:          id,
+		assign:      assign.DeepCopy(),
+		assignValue: value,
+		bindings:    gvks,
+		path:        path,
+		tester:      tester,
+		valueTest:   &valueTests,
 	}, nil
 }
 

pkg/mutation/mutators/assignmeta_mutator.go

@@ -42,9 +42,12 @@ var (
 // AssignMeta instance.
 type AssignMetadataMutator struct {
 	id             types.ID
-	tester         *tester.Tester
 	assignMetadata *mutationsv1alpha1.AssignMetadata
-	path           parser.Path
+	assignValue    string
+
+	path parser.Path
+
+	tester *tester.Tester
 }
 
 // assignMetadataMutator implements mutator.
@@ -60,6 +63,10 @@ func (m *AssignMetadataMutator) Matches(obj client.Object, ns *corev1.Namespace)
 }
 
 func (m *AssignMetadataMutator) Mutate(obj *unstructured.Unstructured) (bool, error) {
+	// Note: Performance here can be improved by ~3x by writing a specialized
+	// function instead of using a generic function. AssignMetadata only ever
+	// mutates metadata.annotations or metadata.labels, and we spend ~70% of
+	// compute covering cases that aren't valid for this Mutator.
 	return core.Mutate(m, m.tester, nil, obj)
 }
 
@@ -91,24 +98,16 @@ func (m *AssignMetadataMutator) HasDiff(mutator types.Mutator) bool {
 func (m *AssignMetadataMutator) DeepCopy() types.Mutator {
 	res := &AssignMetadataMutator{
 		id:             m.id,
-		tester:         m.tester.DeepCopy(),
 		assignMetadata: m.assignMetadata.DeepCopy(),
+		assignValue:    m.assignValue,
 		path:           m.path.DeepCopy(),
+		tester:         m.tester.DeepCopy(),
 	}
 	return res
 }
 
 func (m *AssignMetadataMutator) Value() (interface{}, error) {
-	value, err := types.UnmarshalValue(m.assignMetadata.Spec.Parameters.Assign.Raw)
-	if err != nil {
-		return nil, err
-	}
-	switch t := value.(type) {
-	case string:
-		return value, nil
-	default:
-		return nil, fmt.Errorf("incorrect value for AssignMetadataMutator. Value must be a string. Value: %s Type: %s", value, t)
-	}
+	return m.assignValue, nil
 }
 
 func (m *AssignMetadataMutator) String() string {
@@ -134,7 +133,8 @@ func MutatorForAssignMetadata(assignMeta *mutationsv1alpha1.AssignMetadata) (*As
 	if !ok {
 		return nil, errors.New("spec.parameters.assign must have a string value field for AssignMetadata " + assignMeta.GetName())
 	}
-	if _, ok := value.(string); !ok {
+	valueString, isString := value.(string)
+	if !isString {
 		return nil, errors.New("spec.parameters.assign.value field must be a string for AssignMetadata " + assignMeta.GetName())
 	}
 
@@ -147,9 +147,10 @@ func MutatorForAssignMetadata(assignMeta *mutationsv1alpha1.AssignMetadata) (*As
 
 	return &AssignMetadataMutator{
 		id:             types.MakeID(assignMeta),
-		tester:         t,
 		assignMetadata: assignMeta.DeepCopy(),
+		assignValue:    valueString,
 		path:           path,
+		tester:         t,
 	}, nil
 }