merge: Add flag to skip comparison after apply

Currently we have an expensive (especially since the parent commit)
comparison after applying an object. This is mostly unuseful considering
that we have a piece of code in kubernetes that removes entries that are
no-ops.

Provide a new flag so that the comparison can be disabled, especially
since the comparison is done somewhere else, and the comparison is not
good enough anyway.

```
BenchmarkOperations/Pod/ApplyTwice-8				             267	    437527 ns/op	  110382 B/op	    2722 allocs/op
BenchmarkOperations/Pod/ApplyTwiceNoCompare-8         	                     289	    412029 ns/op	  106516 B/op	    2619 allocs/op
BenchmarkOperations/Node/ApplyTwice-8                 	                     176	    828565 ns/op	  155376 B/op	    3557 allocs/op
BenchmarkOperations/Node/ApplyTwiceNoCompare-8        	                     100	   1179327 ns/op	  149046 B/op	    3370 allocs/op
BenchmarkOperations/Endpoints/ApplyTwice-8            	                      50	   2268639 ns/op	  208438 B/op	    4402 allocs/op
BenchmarkOperations/Endpoints/ApplyTwiceNoCompare-8   	                     126	    921992 ns/op	  110419 B/op	    2347 allocs/op
BenchmarkOperations/Node100%override/ApplyTwice-8     	                     174	    669942 ns/op	  155305 B/op	    3556 allocs/op
BenchmarkOperations/Node100%override/ApplyTwiceNoCompare-8         	     190	    634832 ns/op	  149025 B/op	    3369 allocs/op
BenchmarkOperations/Node10%override/ApplyTwice-8                   	     157	    691645 ns/op	  155316 B/op	    3557 allocs/op
BenchmarkOperations/Node10%override/ApplyTwiceNoCompare-8          	     186	    635949 ns/op	  149036 B/op	    3370 allocs/op
BenchmarkOperations/Endpoints100%override/ApplyTwice-8             	      63	   1811443 ns/op	  208261 B/op	    4400 allocs/op
BenchmarkOperations/Endpoints100%override/ApplyTwiceNoCompare-8    	     127	    932614 ns/op	  110431 B/op	    2347 allocs/op
BenchmarkOperations/Endpoints10%override/ApplyTwice-8              	      58	   1903112 ns/op	  208359 B/op	    4401 allocs/op
BenchmarkOperations/Endpoints10%override/ApplyTwiceNoCompare-8     	     124	    957186 ns/op	  110426 B/op	    2347 allocs/op
BenchmarkOperations/PrometheusCRD/ApplyTwice-8                     	      30	   3843649 ns/op	  983421 B/op	   21105 allocs/op
BenchmarkOperations/PrometheusCRD/ApplyTwiceNoCompare-8            	      34	   3500118 ns/op	  935164 B/op	   19987 allocs/op
BenchmarkOperations/apiresourceimport/ApplyTwice-8                 	       7	  15743401 ns/op	 3650892 B/op	   83842 allocs/op
BenchmarkOperations/apiresourceimport/ApplyTwiceNoCompare-8        	       7	  15123009 ns/op	 3566088 B/op	   81776 allocs/op
```

Author: apelisse

internal/fixture/state.go

@@ -537,6 +537,9 @@ type TestCase struct {
 	Managed fieldpath.ManagedFields
 	// Set to true if the test case needs the union behavior enabled.
 	RequiresUnions bool
+	// ReportInputOnNoop if we don't want to compare the output and
+	// always return it.
+	ReturnInputOnNoop bool
 	// IgnoredFields containing the set to ignore for every version
 	IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set
 }
@@ -569,13 +572,16 @@ func (tc TestCase) PreprocessOperations(parser Parser) error {
 // TestWithConverter once and reset the benchmark, to make sure the test case
 // actually passes..
 func (tc TestCase) BenchWithConverter(parser Parser, converter merge.Converter) error {
+	updaterBuilder := merge.UpdaterBuilder{
+		Converter:         converter,
+		IgnoredFields:     tc.IgnoredFields,
+		ReturnInputOnNoop: tc.ReturnInputOnNoop,
+		EnableUnions:      tc.RequiresUnions,
+	}
 	state := State{
-		Updater: &merge.Updater{Converter: converter, IgnoredFields: tc.IgnoredFields},
+		Updater: updaterBuilder.BuildUpdater(),
 		Parser:  parser,
 	}
-	if tc.RequiresUnions {
-		state.Updater.EnableUnionFeature()
-	}
 	// We currently don't have any test that converts, we can take
 	// care of that later.
 	for i, ops := range tc.Ops {
@@ -589,13 +595,16 @@ func (tc TestCase) BenchWithConverter(parser Parser, converter merge.Converter)
 
 // TestWithConverter runs the test-case using the given parser and converter.
 func (tc TestCase) TestWithConverter(parser Parser, converter merge.Converter) error {
+	updaterBuilder := merge.UpdaterBuilder{
+		Converter:         converter,
+		IgnoredFields:     tc.IgnoredFields,
+		ReturnInputOnNoop: tc.ReturnInputOnNoop,
+		EnableUnions:      tc.RequiresUnions,
+	}
 	state := State{
-		Updater: &merge.Updater{Converter: converter, IgnoredFields: tc.IgnoredFields},
+		Updater: updaterBuilder.BuildUpdater(),
 		Parser:  parser,
 	}
-	if tc.RequiresUnions {
-		state.Updater.EnableUnionFeature()
-	}
 	for i, ops := range tc.Ops {
 		err := ops.run(&state)
 		if err != nil {

merge/real_test.go

@@ -108,8 +108,9 @@ func BenchmarkOperations(b *testing.B) {
 		b.Run(bench.name, func(b *testing.B) {
 			obj := typed.YAMLObject(read(testdata(bench.filename)))
 			tests := []struct {
-				name string
-				ops  []Operation
+				name              string
+				returnInputonNoop bool
+				ops               []Operation
 			}{
 				{
 					name: "Create",
@@ -146,6 +147,22 @@ func BenchmarkOperations(b *testing.B) {
 						},
 					},
 				},
+				{
+					name:              "ApplyTwiceNoCompare",
+					returnInputonNoop: true,
+					ops: []Operation{
+						Apply{
+							Manager:    "controller",
+							APIVersion: "v1",
+							Object:     obj,
+						},
+						Apply{
+							Manager:    "other-controller",
+							APIVersion: "v1",
+							Object:     obj,
+						},
+					},
+				},
 				{
 					name: "Update",
 					ops: []Operation{
@@ -180,7 +197,8 @@ func BenchmarkOperations(b *testing.B) {
 			for _, test := range tests {
 				b.Run(test.name, func(b *testing.B) {
 					tc := TestCase{
-						Ops: test.ops,
+						Ops:               test.ops,
+						ReturnInputOnNoop: test.returnInputonNoop,
 					}
 					p := SameVersionParser{T: bench.parseType}
 					tc.PreprocessOperations(p)

merge/update.go

@@ -28,17 +28,50 @@ type Converter interface {
 	IsMissingVersionError(error) bool
 }
 
+// UpdateBuilder allows you to create a new Updater by exposing all of
+// the options and setting them once.
+type UpdaterBuilder struct {
+	Converter     Converter
+	IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set
+
+	EnableUnions bool
+
+	// Stop comparing the new object with old object after applying.
+	// This was initially used to avoid spurious etcd update, but
+	// since that's vastly inefficient, we've come-up with a better
+	// way of doing that. Create this flag to stop it.
+	// Comparing has become more expensive too now that we're not using
+	// `Compare` but `value.Equals` so this gives an option to avoid it.
+	ReturnInputOnNoop bool
+}
+
+func (u *UpdaterBuilder) BuildUpdater() *Updater {
+	return &Updater{
+		Converter:         u.Converter,
+		IgnoredFields:     u.IgnoredFields,
+		enableUnions:      u.EnableUnions,
+		returnInputOnNoop: u.ReturnInputOnNoop,
+	}
+}
+
 // Updater is the object used to compute updated FieldSets and also
 // merge the object on Apply.
 type Updater struct {
-	Converter     Converter
+	// Deprecated: This will eventually become private.
+	Converter Converter
+
+	// Deprecated: This will eventually become private.
 	IgnoredFields map[fieldpath.APIVersion]*fieldpath.Set
 
 	enableUnions bool
+
+	returnInputOnNoop bool
 }
 
 // EnableUnionFeature turns on union handling. It is disabled by default until the
 // feature is complete.
+//
+// Deprecated: Use the builder instead.
 func (s *Updater) EnableUnionFeature() {
 	s.enableUnions = true
 }
@@ -204,7 +237,7 @@ func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fiel
 	if err != nil {
 		return nil, fieldpath.ManagedFields{}, err
 	}
-	if value.EqualsUsing(value.NewFreelistAllocator(), liveObject.AsValue(), newObject.AsValue()) {
+	if !s.returnInputOnNoop && value.EqualsUsing(value.NewFreelistAllocator(), liveObject.AsValue(), newObject.AsValue()) {
 		newObject = nil
 	}
 	return newObject, managers, nil