From 3b4474b5743890a51035552af362f31b93bc1e5b Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Tue, 9 Jan 2024 10:25:15 -0500 Subject: [PATCH] Reduce CacheImpl lock contention --- .../informers/cache/BasicItemStore.java | 5 +- .../informers/impl/cache/CacheImpl.java | 176 +++++++++++------- 2 files changed, 114 insertions(+), 67 deletions(-) diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/informers/cache/BasicItemStore.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/informers/cache/BasicItemStore.java index 3e878792624..8d3b016b8fb 100644 --- a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/informers/cache/BasicItemStore.java +++ b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/informers/cache/BasicItemStore.java @@ -19,13 +19,14 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.function.Function; import java.util.stream.Stream; public class BasicItemStore implements ItemStore { - private Function keyFunction; - private ConcurrentHashMap store = new ConcurrentHashMap<>(); + private final Function keyFunction; + private final ConcurrentMap store = new ConcurrentHashMap<>(); public BasicItemStore(Function keyFunction) { this.keyFunction = keyFunction; diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/informers/impl/cache/CacheImpl.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/informers/impl/cache/CacheImpl.java index a9404dd1c54..c670c385be2 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/informers/impl/cache/CacheImpl.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/informers/impl/cache/CacheImpl.java @@ -24,13 +24,14 @@ import io.fabric8.kubernetes.client.utils.Utils; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.function.Function; import java.util.stream.Collectors; @@ -44,13 +45,14 @@ public class CacheImpl implements Cache { public static final String NAMESPACE_INDEX = "namespace"; // indexers stores index functions by their names - private final Map>> indexers = new HashMap<>(); + private final Map>> indexers = Collections.synchronizedMap(new HashMap<>()); // items stores object instances + // @GuardedBy("getLockObj") private ItemStore items; // indices stores objects' key by their indices - private final Map>> indices = new HashMap<>(); + private final ConcurrentMap>> indices = new ConcurrentHashMap<>(); public CacheImpl() { this(NAMESPACE_INDEX, Cache::metaNamespaceIndexFunc, Cache::metaNamespaceKeyFunc); @@ -71,12 +73,12 @@ public void setItemStore(ItemStore items) { * @return registered indexers */ @Override - public synchronized Map>> getIndexers() { + public Map>> getIndexers() { return Collections.unmodifiableMap(indexers); } @Override - public synchronized void addIndexers(Map>> indexersNew) { + public void addIndexers(Map>> indexersNew) { Set intersection = new HashSet<>(indexers.keySet()); intersection.retainAll(indexersNew.keySet()); if (!intersection.isEmpty()) { @@ -94,12 +96,15 @@ public synchronized void addIndexers(Map>> inde * @param obj the object * @return the old object */ - public synchronized T put(T obj) { + public T put(T obj) { if (obj == null) { return null; } String key = getKey(obj); - T oldObj = this.items.put(key, obj); + T oldObj; + synchronized (getLockObject()) { + oldObj = this.items.put(key, obj); + } this.updateIndices(oldObj, obj, key); return oldObj; } @@ -110,9 +115,12 @@ public synchronized T put(T obj) { * @param obj object * @return the old object */ - public synchronized T remove(T obj) { + public T remove(T obj) { String key = getKey(obj); - T old = this.items.remove(key); + T old; + synchronized (getLockObject()) { + old = this.items.remove(key); + } if (old != null) { this.deleteFromIndices(old, key); } @@ -126,7 +134,9 @@ public synchronized T remove(T obj) { */ @Override public List listKeys() { - return this.items.keySet().collect(Collectors.toList()); + synchronized (getLockObject()) { + return this.items.keySet().collect(Collectors.toList()); + } } /** @@ -146,8 +156,10 @@ public T get(T obj) { */ @Override public String getKey(T obj) { - String result = this.items.getKey(obj); - return result == null ? "" : result; + synchronized (getLockObject()) { + String result = this.items.getKey(obj); + return result == null ? "" : result; + } } /** @@ -157,7 +169,9 @@ public String getKey(T obj) { */ @Override public List list() { - return this.items.values().collect(Collectors.toList()); + synchronized (getLockObject()) { + return this.items.values().collect(Collectors.toList()); + } } /** @@ -168,7 +182,9 @@ public List list() { */ @Override public T getByKey(String key) { - return this.items.get(key); + synchronized (getLockObject()) { + return this.items.get(key); + } } /** @@ -179,17 +195,17 @@ public T getByKey(String key) { * @return the list */ @Override - public synchronized List index(String indexName, T obj) { - if (!this.indexers.containsKey(indexName)) { + public List index(String indexName, T obj) { + Function> indexFunc = this.indexers.get(indexName); + if (indexFunc == null) { throw new IllegalArgumentException(String.format("index %s doesn't exist!", indexName)); } - Function> indexFunc = this.indexers.get(indexName); - List indexKeys = indexFunc.apply(obj); Map> index = this.indices.get(indexName); if (index.isEmpty()) { return new ArrayList<>(); } + List indexKeys = indexFunc.apply(obj); Set returnKeySet = new HashSet<>(); for (String indexKey : indexKeys) { Set set = index.get(indexKey); @@ -201,11 +217,23 @@ public synchronized List index(String indexName, T obj) { List items = new ArrayList<>(returnKeySet.size()); for (String absoluteKey : returnKeySet) { - items.add(this.items.get(absoluteKey)); + T item; + synchronized (getLockObject()) { + item = this.items.get(absoluteKey); + } + if (item != null) { + items.add(item); + } } return items; } + private void checkContainsIndex(String indexName) { + if (!this.indexers.containsKey(indexName)) { + throw new IllegalArgumentException(String.format("index %s doesn't exist!", indexName)); + } + } + /** * Index keys list * @@ -214,17 +242,14 @@ public synchronized List index(String indexName, T obj) { * @return the list */ @Override - public synchronized List indexKeys(String indexName, String indexKey) { - if (!this.indexers.containsKey(indexName)) { - throw new IllegalArgumentException(String.format("index %s doesn't exist!", indexName)); - } + public List indexKeys(String indexName, String indexKey) { + checkContainsIndex(indexName); Map> index = this.indices.get(indexName); - Set set = index.get(indexKey); - List keys = new ArrayList<>(set.size()); - for (String key : set) { - keys.add(key); + if (index == null) { + return new ArrayList<>(); } - return keys; + Set set = index.getOrDefault(indexKey, Collections.emptySet()); + return new ArrayList<>(set); } /** @@ -235,18 +260,25 @@ public synchronized List indexKeys(String indexName, String indexKey) { * @return the list */ @Override - public synchronized List byIndex(String indexName, String indexKey) { - if (!this.indexers.containsKey(indexName)) { - throw new IllegalArgumentException(String.format("index %s doesn't exist!", indexName)); - } + public List byIndex(String indexName, String indexKey) { + checkContainsIndex(indexName); Map> index = this.indices.get(indexName); + if (index == null) { + return new ArrayList<>(); + } Set set = index.get(indexKey); if (set == null) { - return Arrays.asList(); + return new ArrayList<>(); } List items = new ArrayList<>(set.size()); for (String key : set) { - items.add(this.items.get(key)); + T item; + synchronized (getLockObject()) { + item = this.items.get(key); + } + if (item != null) { + items.add(item); + } } return items; } @@ -265,12 +297,15 @@ void updateIndices(T oldObj, T newObj, String key) { deleteFromIndices(oldObj, key); } - for (Map.Entry>> indexEntry : indexers.entrySet()) { - String indexName = indexEntry.getKey(); - Function> indexFunc = indexEntry.getValue(); - Map> index = this.indices.get(indexName); - - updateIndex(key, newObj, indexFunc, index); + synchronized (indexers) { + for (Map.Entry>> indexEntry : indexers.entrySet()) { + String indexName = indexEntry.getKey(); + Function> indexFunc = indexEntry.getValue(); + Map> index = this.indices.get(indexName); + if (index != null) { + updateIndex(key, newObj, indexFunc, index); + } + } } } @@ -278,8 +313,9 @@ private void updateIndex(String key, T newObj, Function> indexFu List indexValues = indexFunc.apply(newObj); if (indexValues != null && !indexValues.isEmpty()) { for (String indexValue : indexValues) { - Set indexSet = index.computeIfAbsent(indexValue, k -> new HashSet<>()); - indexSet.add(key); + if (indexValue != null) { + index.computeIfAbsent(indexValue, k -> ConcurrentHashMap.newKeySet()).add(key); + } } } } @@ -293,21 +329,25 @@ private void updateIndex(String key, T newObj, Function> indexFu * @param key the key */ private void deleteFromIndices(T oldObj, String key) { - for (Map.Entry>> indexEntry : this.indexers.entrySet()) { - Function> indexFunc = indexEntry.getValue(); - List indexValues = indexFunc.apply(oldObj); - if (indexValues == null || indexValues.isEmpty()) { - continue; - } + synchronized (indexers) { + for (Map.Entry>> indexEntry : this.indexers.entrySet()) { + Function> indexFunc = indexEntry.getValue(); + List indexValues = indexFunc.apply(oldObj); + if (indexValues == null || indexValues.isEmpty()) { + continue; + } - Map> index = this.indices.get(indexEntry.getKey()); - if (index == null) { - continue; - } - for (String indexValue : indexValues) { - Set indexSet = index.get(indexValue); - if (indexSet != null) { - indexSet.remove(key); + Map> index = this.indices.get(indexEntry.getKey()); + if (index == null) { + continue; + } + for (String indexValue : indexValues) { + if (indexValue != null) { + Set indexSet = index.get(indexValue); + if (indexSet != null) { + indexSet.remove(key); + } + } } } } @@ -319,12 +359,16 @@ private void deleteFromIndices(T oldObj, String key) { * @param indexName the index name * @param indexFunc the index func */ - public synchronized CacheImpl addIndexFunc(String indexName, Function> indexFunc) { - HashMap> index = new HashMap<>(); - this.indices.put(indexName, index); - this.indexers.put(indexName, indexFunc); - - items.values().forEach(v -> updateIndex(getKey(v), v, indexFunc, index)); + public CacheImpl addIndexFunc(String indexName, Function> indexFunc) { + ConcurrentMap> index = new ConcurrentHashMap<>(); + synchronized (indexers) { + this.indices.put(indexName, index); + this.indexers.put(indexName, indexFunc); + + synchronized (getLockObject()) { + items.values().forEach(v -> updateIndex(getKey(v), v, indexFunc, index)); + } + } return this; } @@ -387,17 +431,19 @@ public static List metaNamespaceIndexFunc(Object obj) { } @Override - public synchronized void removeIndexer(String name) { + public void removeIndexer(String name) { this.indices.remove(name); this.indexers.remove(name); } public boolean isFullState() { - return items.isFullState(); + synchronized (getLockObject()) { + return items.isFullState(); + } } public Object getLockObject() { - return this; + return this.items; } }