Avoid deep copying objects twice in the CacheReader List method

[charith-elastic]

In the List method of the CacheReader, the list of objects retrieved from the index is added to the runtimeObjs array by deep copying each object. This array is then passed to the objectutil.FilterWithLabels which again creates a deep copy of the objects that match the label selector.

It would be more efficient to inline the label filtering logic as follows to avoid this extra work and reduce heap usage of controllers that work with large number of items.

diff --git a/pkg/cache/internal/cache_reader.go b/pkg/cache/internal/cache_reader.go
index a9b7ae34..8564a2da 100644
--- a/pkg/cache/internal/cache_reader.go
+++ b/pkg/cache/internal/cache_reader.go
@@ -30,7 +30,6 @@ import (
 	"k8s.io/apimachinery/pkg/selection"
 	"k8s.io/client-go/tools/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/internal/objectutil"
 )
 
 // CacheReader is a CacheReader
@@ -125,15 +124,22 @@ func (c *CacheReader) List(_ context.Context, out runtime.Object, opts ...client
 		if !isObj {
 			return fmt.Errorf("cache contained %T, which is not an Object", obj)
 		}
+		meta, err := apimeta.Accessor(obj)
+		if err != nil {
+			return err
+		}
+		if labelSel != nil {
+			lbls := labels.Set(meta.GetLabels())
+			if !labelSel.Matches(lbls) {
+				continue
+			}
+		}
+
 		outObj := obj.DeepCopyObject()
 		outObj.GetObjectKind().SetGroupVersionKind(c.groupVersionKind)
 		runtimeObjs = append(runtimeObjs, outObj)
 	}
-	filteredItems, err := objectutil.FilterWithLabels(runtimeObjs, labelSel)
-	if err != nil {
-		return err
-	}
-	return apimeta.SetList(out, filteredItems)
+	return apimeta.SetList(out, runtimeObjs)
 }
 
 // objectKeyToStorageKey converts an object key to store key.

Are there any downsides to patching the method as suggested? If not, I'll be happy to raise a PR.

[mengqiy]

controller-runtime/pkg/cache/internal/cache_reader.go

Lines 122 to 136 in 801e12a

	runtimeObjs := make([]runtime.Object, 0, len(objs))
	for _, item := range objs {
	obj, isObj := item.(runtime.Object)
	if !isObj {
	return fmt.Errorf("cache contained %T, which is not an Object", obj)
	}
	outObj := obj.DeepCopyObject()
	outObj.GetObjectKind().SetGroupVersionKind(c.groupVersionKind)
	runtimeObjs = append(runtimeObjs, outObj)
	}
	filteredItems, err := objectutil.FilterWithLabels(runtimeObjs, labelSel)
	if err != nil {
	return err
	}
	return apimeta.SetList(out, filteredItems)

We deepcopy twice currently. One in the for loop, the other one in FilterWithLabels.
I believe we can remove one deepcopy.
What you propose makes sense to me overall, though we need to agree on the implementation details.

Also be aware this potential issue: https://github.com/kubernetes-sigs/controller-runtime/pull/389/files#r274350644

cc: @DirectXMan12

[charith-elastic]

I saw the comments in #389 too. When I ran the tests with the proposed change, there were no race detector warnings so I don't think that will be an issue.

[mengqiy]

Cool! Feel free to open a PR.