From e4f74dbbebfe753d2b9a45b6d1c661697548f0e8 Mon Sep 17 00:00:00 2001
From: Kinara Shah <kinara@rancher.com>
Date: Mon, 28 Mar 2022 09:51:56 -0700
Subject: [PATCH] Revert "Optimize index updating"

This reverts commit e61d0d5d52a5f9ed1a1ea453f2e831f604f44fed.
---
 tools/cache/thread_safe_store.go      | 91 +++++++++++----------------
 tools/cache/thread_safe_store_test.go |  3 +
 2 files changed, 41 insertions(+), 53 deletions(-)

diff --git a/tools/cache/thread_safe_store.go b/tools/cache/thread_safe_store.go
index f8f084673c..a86873a56c 100644
--- a/tools/cache/thread_safe_store.go
+++ b/tools/cache/thread_safe_store.go
@@ -90,7 +90,7 @@ func (c *threadSafeMap) Delete(key string) {
 	c.lock.Lock()
 	defer c.lock.Unlock()
 	if obj, exists := c.items[key]; exists {
-		c.updateIndices(obj, nil, key)
+		c.deleteFromIndices(obj, key)
 		delete(c.items, key)
 	}
 }
@@ -257,76 +257,61 @@ func (c *threadSafeMap) AddIndexers(newIndexers Indexers) error {
 	return nil
 }
 
-// updateIndices modifies the objects location in the managed indexes:
-// - for create you must provide only the newObj
-// - for update you must provide both the oldObj and the newObj
-// - for delete you must provide only the oldObj
+// updateIndices modifies the objects location in the managed indexes, if this is an update, you must provide an oldObj
 // updateIndices must be called from a function that already has a lock on the cache
 func (c *threadSafeMap) updateIndices(oldObj interface{}, newObj interface{}, key string) {
-	var oldIndexValues, indexValues []string
-	var err error
+	// if we got an old object, we need to remove it before we add it again
+	if oldObj != nil {
+		c.deleteFromIndices(oldObj, key)
+	}
 	for name, indexFunc := range c.indexers {
-		if oldObj != nil {
-			oldIndexValues, err = indexFunc(oldObj)
-		} else {
-			oldIndexValues = oldIndexValues[:0]
-		}
+		indexValues, err := indexFunc(newObj)
 		if err != nil {
 			panic(fmt.Errorf("unable to calculate an index entry for key %q on index %q: %v", key, name, err))
 		}
-
-		if newObj != nil {
-			indexValues, err = indexFunc(newObj)
-		} else {
-			indexValues = indexValues[:0]
-		}
-		if err != nil {
-			panic(fmt.Errorf("unable to calculate an index entry for key %q on index %q: %v", key, name, err))
-		}
-
 		index := c.indices[name]
 		if index == nil {
 			index = Index{}
 			c.indices[name] = index
 		}
 
-		for _, value := range oldIndexValues {
-			// We optimize for the most common case where index returns a single value.
-			if len(indexValues) == 1 && value == indexValues[0] {
-				continue
+		for _, indexValue := range indexValues {
+			set := index[indexValue]
+			if set == nil {
+				set = sets.String{}
+				index[indexValue] = set
 			}
-			c.deleteKeyFromIndex(key, value, index)
-		}
-		for _, value := range indexValues {
-			// We optimize for the most common case where index returns a single value.
-			if len(oldIndexValues) == 1 && value == oldIndexValues[0] {
-				continue
-			}
-			c.addKeyToIndex(key, value, index)
+			set.Insert(key)
 		}
 	}
 }
 
-func (c *threadSafeMap) addKeyToIndex(key, indexValue string, index Index) {
-	set := index[indexValue]
-	if set == nil {
-		set = sets.String{}
-		index[indexValue] = set
-	}
-	set.Insert(key)
-}
+// deleteFromIndices removes the object from each of the managed indexes
+// it is intended to be called from a function that already has a lock on the cache
+func (c *threadSafeMap) deleteFromIndices(obj interface{}, key string) {
+	for name, indexFunc := range c.indexers {
+		indexValues, err := indexFunc(obj)
+		if err != nil {
+			panic(fmt.Errorf("unable to calculate an index entry for key %q on index %q: %v", key, name, err))
+		}
 
-func (c *threadSafeMap) deleteKeyFromIndex(key, indexValue string, index Index) {
-	set := index[indexValue]
-	if set == nil {
-		return
-	}
-	set.Delete(key)
-	// If we don't delete the set when zero, indices with high cardinality
-	// short lived resources can cause memory to increase over time from
-	// unused empty sets. See `kubernetes/kubernetes/issues/84959`.
-	if len(set) == 0 {
-		delete(index, indexValue)
+		index := c.indices[name]
+		if index == nil {
+			continue
+		}
+		for _, indexValue := range indexValues {
+			set := index[indexValue]
+			if set != nil {
+				set.Delete(key)
+
+				// If we don't delete the set when zero, indices with high cardinality
+				// short lived resources can cause memory to increase over time from
+				// unused empty sets. See `kubernetes/kubernetes/issues/84959`.
+				if len(set) == 0 {
+					delete(index, indexValue)
+				}
+			}
+		}
 	}
 }
 
diff --git a/tools/cache/thread_safe_store_test.go b/tools/cache/thread_safe_store_test.go
index 161a5ddcd8..9cf8d3a759 100644
--- a/tools/cache/thread_safe_store_test.go
+++ b/tools/cache/thread_safe_store_test.go
@@ -89,6 +89,9 @@ func TestThreadSafeStoreAddKeepsNonEmptySetPostDeleteFromIndex(t *testing.T) {
 	}
 }
 
+// BenchmarkIndexer-12    	  447849	      2738 ns/op	     242 B/op	       4 allocs/op
+// PASS
+// ok  	k8s.io/kubernetes/vendor/k8s.io/client-go/tools/cache	2.451s
 func BenchmarkIndexer(b *testing.B) {
 	testIndexer := "testIndexer"