From b4595bc60298c9c5abc4df8bf185c11de5989c75 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Thu, 9 Sep 2021 12:01:29 +0200 Subject: [PATCH 1/2] Make Cache More Memory Efficient No need to keep counters per segment when we only use them in aggregate. Also, removed a few spots of using needless indirection by making segments reference the cache itself directly and saved a bunch of empty maps for idle caches on e.g. rarely used indices. --- .../org/elasticsearch/common/cache/Cache.java | 231 +++++++++--------- 1 file changed, 113 insertions(+), 118 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/cache/Cache.java b/server/src/main/java/org/elasticsearch/common/cache/Cache.java index f0399d25b64a2..e72f00be6e147 100644 --- a/server/src/main/java/org/elasticsearch/common/cache/Cache.java +++ b/server/src/main/java/org/elasticsearch/common/cache/Cache.java @@ -11,7 +11,7 @@ import org.elasticsearch.core.Tuple; import org.elasticsearch.common.util.concurrent.ReleasableLock; -import java.util.Arrays; +import java.lang.reflect.Array; import java.util.HashMap; import java.util.Iterator; import java.util.Map; @@ -25,7 +25,6 @@ import java.util.function.BiConsumer; import java.util.function.BiFunction; import java.util.function.Consumer; -import java.util.function.Predicate; import java.util.function.ToLongBiFunction; /** @@ -59,6 +58,12 @@ */ public class Cache { + private final LongAdder hits = new LongAdder(); + + private final LongAdder misses = new LongAdder(); + + private final LongAdder evictions = new LongAdder(); + // positive if entries have an expiration private long expireAfterAccessNanos = -1; @@ -150,10 +155,10 @@ enum State { NEW, EXISTING, DELETED } - static class Entry { + private static final class Entry { final K key; final V value; - long writeTime; + final long writeTime; volatile long accessTime; Entry before; Entry after; @@ -170,20 +175,15 @@ static class Entry { * A cache segment. *

* A CacheSegment is backed by a HashMap and is protected by a read/write lock. - * - * @param the type of the keys - * @param the type of the values */ - private static class CacheSegment { + private final class CacheSegment { // read/write lock protecting mutations to the segment ReadWriteLock segmentLock = new ReentrantReadWriteLock(); ReleasableLock readLock = new ReleasableLock(segmentLock.readLock()); ReleasableLock writeLock = new ReleasableLock(segmentLock.writeLock()); - Map>> map = new HashMap<>(); - - SegmentStats segmentStats = new SegmentStats(); + Map>> map; /** * get an entry from the segment; expired entries will be returned as null but not removed from the cache until the LRU list is @@ -191,14 +191,13 @@ private static class CacheSegment { * * @param key the key of the entry to get from the cache * @param now the access time of this entry - * @param isExpired test if the entry is expired - * @param onExpiration a callback if the entry associated to the key is expired + * @param eagerEvict whether entries should be eagerly evicted on expiration * @return the entry if there was one, otherwise null */ - Entry get(K key, long now, Predicate> isExpired, Consumer> onExpiration) { + Entry get(K key, long now, boolean eagerEvict) { CompletableFuture> future; try (ReleasableLock ignored = readLock.acquire()) { - future = map.get(key); + future = map == null ? null : map.get(key); } if (future != null) { Entry entry; @@ -206,22 +205,26 @@ Entry get(K key, long now, Predicate> isExpired, Consumer, Entry> put(K key, V value, long now) { Entry existing = null; try (ReleasableLock ignored = writeLock.acquire()) { try { + if (map == null) { + map = new HashMap<>(); + } CompletableFuture> future = map.put(key, CompletableFuture.completedFuture(entry)); if (future != null) { - existing = future.handle((ok, ex) -> { - if (ok != null) { - return ok; - } else { - return null; - } - }).get(); + existing = future.handle((ok, ex) -> ok).get(); } } catch (ExecutionException | InterruptedException e) { throw new IllegalStateException(e); @@ -260,16 +260,22 @@ Tuple, Entry> put(K key, V value, long now) { * remove an entry from the segment * * @param key the key of the entry to remove from the cache - * @param onRemoval a callback for the removed entry */ - void remove(K key, Consumer>> onRemoval) { + void remove(K key) { CompletableFuture> future; try (ReleasableLock ignored = writeLock.acquire()) { - future = map.remove(key); + if (map == null) { + future = null; + } else { + future = map.remove(key); + if (map.isEmpty()) { + map = null; + } + } } if (future != null) { - segmentStats.eviction(); - onRemoval.accept(future); + evictions.increment(); + notifyWithInvalidated(future); } } @@ -279,19 +285,22 @@ void remove(K key, Consumer>> onRemoval) { * * @param key the key of the entry to remove from the cache * @param value the value expected to be associated with the key - * @param onRemoval a callback for the removed entry + * @param notify whether to trigger a removal notification if the entry has been removed */ - void remove(K key, V value, Consumer>> onRemoval) { + void remove(K key, V value, boolean notify) { CompletableFuture> future; boolean removed = false; try (ReleasableLock ignored = writeLock.acquire()) { - future = map.get(key); + future = map == null ? null : map.get(key); try { if (future != null) { if (future.isDone()) { Entry entry = future.get(); if (Objects.equals(value, entry.value)) { removed = map.remove(key, future); + if (map.isEmpty()) { + map = null; + } } } } @@ -301,37 +310,22 @@ void remove(K key, V value, Consumer>> onRemoval) } if (future != null && removed) { - segmentStats.eviction(); - onRemoval.accept(future); - } - } - - private static class SegmentStats { - private final LongAdder hits = new LongAdder(); - private final LongAdder misses = new LongAdder(); - private final LongAdder evictions = new LongAdder(); - - void hit() { - hits.increment(); - } - - void miss() { - misses.increment(); - } - - void eviction() { evictions.increment(); + if (notify) { + notifyWithInvalidated(future); + } } } + } public static final int NUMBER_OF_SEGMENTS = 256; - @SuppressWarnings({"unchecked", "rawtypes"}) - private final CacheSegment[] segments = new CacheSegment[NUMBER_OF_SEGMENTS]; + @SuppressWarnings("unchecked") + private final CacheSegment[] segments = (CacheSegment[]) Array.newInstance(CacheSegment.class, NUMBER_OF_SEGMENTS); { for (int i = 0; i < segments.length; i++) { - segments[i] = new CacheSegment<>(); + segments[i] = new CacheSegment(); } } @@ -348,12 +342,12 @@ void eviction() { * @return the value to which the specified key is mapped, or null if this map contains no mapping for the key */ public V get(K key) { - return get(key, now(), e -> {}); + return get(key, now(), false); } - private V get(K key, long now, Consumer> onExpiration) { - CacheSegment segment = getCacheSegment(key); - Entry entry = segment.get(key, now, e -> isExpired(e, now), onExpiration); + private V get(K key, long now, boolean eagerEvict) { + CacheSegment segment = getCacheSegment(key); + Entry entry = segment.get(key, now, eagerEvict); if (entry == null) { return null; } else { @@ -379,36 +373,36 @@ private V get(K key, long now, Consumer> onExpiration) { public V computeIfAbsent(K key, CacheLoader loader) throws ExecutionException { long now = now(); // we have to eagerly evict expired entries or our putIfAbsent call below will fail - V value = get(key, now, e -> { - try (ReleasableLock ignored = lruLock.acquire()) { - evictEntry(e); - } - }); + V value = get(key, now, true); if (value == null) { // we need to synchronize loading of a value for a given key; however, holding the segment lock while // invoking load can lead to deadlock against another thread due to dependent key loading; therefore, we // need a mechanism to ensure that load is invoked at most once, but we are not invoking load while holding // the segment lock; to do this, we atomically put a future in the map that can load the value, and then // get the value from this future on the thread that won the race to place the future into the segment map - CacheSegment segment = getCacheSegment(key); + CacheSegment segment = getCacheSegment(key); CompletableFuture> future; CompletableFuture> completableFuture = new CompletableFuture<>(); try (ReleasableLock ignored = segment.writeLock.acquire()) { + if (segment.map == null) { + segment.map = new HashMap<>(); + } future = segment.map.putIfAbsent(key, completableFuture); } BiFunction, Throwable, ? extends V> handler = (ok, ex) -> { if (ok != null) { - try (ReleasableLock ignored = lruLock.acquire()) { - promote(ok, now); - } + promote(ok, now); return ok.value; } else { try (ReleasableLock ignored = segment.writeLock.acquire()) { - CompletableFuture> sanity = segment.map.get(key); + CompletableFuture> sanity = segment.map == null ? null : segment.map.get(key); if (sanity != null && sanity.isCompletedExceptionally()) { segment.map.remove(key); + if (segment.map.isEmpty()) { + segment.map = null; + } } } return null; @@ -464,7 +458,7 @@ public void put(K key, V value) { } private void put(K key, V value, long now) { - CacheSegment segment = getCacheSegment(key); + CacheSegment segment = getCacheSegment(key); Tuple, Entry> tuple = segment.put(key, value, now); boolean replaced = false; try (ReleasableLock ignored = lruLock.acquire()) { @@ -473,7 +467,7 @@ private void put(K key, V value, long now) { replaced = true; } } - promote(tuple.v1(), now); + promoteNoLock(tuple.v1(), now); } if (replaced) { removalListener.onRemoval(new RemovalNotification<>(tuple.v2().key, tuple.v2().value, @@ -481,7 +475,7 @@ private void put(K key, V value, long now) { } } - private final Consumer>> invalidationConsumer = f -> { + private void notifyWithInvalidated(CompletableFuture> f) { try { Entry entry = f.get(); try (ReleasableLock ignored = lruLock.acquire()) { @@ -492,7 +486,7 @@ private void put(K key, V value, long now) { } catch (InterruptedException e) { throw new IllegalStateException(e); } - }; + } /** * Invalidate the association for the specified key. A removal notification will be issued for invalidated @@ -501,8 +495,8 @@ private void put(K key, V value, long now) { * @param key the key whose mapping is to be invalidated from the cache */ public void invalidate(K key) { - CacheSegment segment = getCacheSegment(key); - segment.remove(key, invalidationConsumer); + CacheSegment segment = getCacheSegment(key); + segment.remove(key); } /** @@ -514,8 +508,8 @@ public void invalidate(K key) { * @param value the expected value that should be associated with the key */ public void invalidate(K key, V value) { - CacheSegment segment = getCacheSegment(key); - segment.remove(key, value, invalidationConsumer); + CacheSegment segment = getCacheSegment(key); + segment.remove(key, value, true); } /** @@ -533,7 +527,9 @@ public void invalidateAll() { } try (ReleasableLock ignored = lruLock.acquire()) { h = head; - Arrays.stream(segments).forEach(segment -> segment.map = new HashMap<>()); + for (CacheSegment segment : segments) { + segment.map = null; + } Entry current = head; while (current != null) { current.state = State.DELETED; @@ -593,7 +589,7 @@ public long weight() { */ public Iterable keys() { return () -> new Iterator() { - private CacheIterator iterator = new CacheIterator(head); + private final CacheIterator iterator = new CacheIterator(head); @Override public boolean hasNext() { @@ -621,7 +617,7 @@ public void remove() { */ public Iterable values() { return () -> new Iterator() { - private CacheIterator iterator = new CacheIterator(head); + private final CacheIterator iterator = new CacheIterator(head); @Override public boolean hasNext() { @@ -649,8 +645,11 @@ public void remove() { * @param consumer the {@link Consumer} */ public void forEach(BiConsumer consumer) { - for (CacheSegment segment : segments) { + for (CacheSegment segment : segments) { try (ReleasableLock ignored = segment.readLock.acquire()) { + if (segment.map == null) { + continue; + } for (CompletableFuture> future : segment.map.values()) { try { if (future != null && future.isDone()) { @@ -690,8 +689,8 @@ public Entry next() { public void remove() { Entry entry = current; if (entry != null) { - CacheSegment segment = getCacheSegment(entry.key); - segment.remove(entry.key, entry.value, f -> {}); + CacheSegment segment = getCacheSegment(entry.key); + segment.remove(entry.key, entry.value, false); try (ReleasableLock ignored = lruLock.acquire()) { current = null; delete(entry, RemovalNotification.RemovalReason.INVALIDATED); @@ -707,21 +706,13 @@ public void remove() { * @return the current cache statistics */ public CacheStats stats() { - long hits = 0; - long misses = 0; - long evictions = 0; - for (int i = 0; i < segments.length; i++) { - hits += segments[i].segmentStats.hits.longValue(); - misses += segments[i].segmentStats.misses.longValue(); - evictions += segments[i].segmentStats.evictions.longValue(); - } - return new CacheStats(hits, misses, evictions); + return new CacheStats(this.hits.sum(), misses.sum(), evictions.sum()); } public static class CacheStats { - private long hits; - private long misses; - private long evictions; + private final long hits; + private final long misses; + private final long evictions; public CacheStats(long hits, long misses, long evictions) { this.hits = hits; @@ -742,25 +733,29 @@ public long getEvictions() { } } - private boolean promote(Entry entry, long now) { - boolean promoted = true; + private void promote(Entry entry, long now) { try (ReleasableLock ignored = lruLock.acquire()) { - switch (entry.state) { - case DELETED: - promoted = false; - break; - case EXISTING: - relinkAtHead(entry); - break; - case NEW: - linkAtHead(entry); - break; - } - if (promoted) { - evict(now); - } + promoteNoLock(entry, now); + } + } + + private void promoteNoLock(Entry entry, long now) { + assert lruLock.isHeldByCurrentThread(); + boolean promoted = true; + switch (entry.state) { + case DELETED: + promoted = false; + break; + case EXISTING: + relinkAtHead(entry); + break; + case NEW: + linkAtHead(entry); + break; + } + if (promoted) { + evict(now); } - return promoted; } private void evict(long now) { @@ -774,9 +769,9 @@ private void evict(long now) { private void evictEntry(Entry entry) { assert lruLock.isHeldByCurrentThread(); - CacheSegment segment = getCacheSegment(entry.key); + CacheSegment segment = getCacheSegment(entry.key); if (segment != null) { - segment.remove(entry.key, entry.value, f -> {}); + segment.remove(entry.key, entry.value, false); } delete(entry, RemovalNotification.RemovalReason.EVICTED); } @@ -871,7 +866,7 @@ private void relinkAtHead(Entry entry) { } } - private CacheSegment getCacheSegment(K key) { + private CacheSegment getCacheSegment(K key) { return segments[key.hashCode() & 0xff]; } } From ccc6282070b065e4c6ec34bc9d357f0aee6e4ae3 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Mon, 13 Sep 2021 09:43:32 +0200 Subject: [PATCH 2/2] inline promotino --- .../org/elasticsearch/common/cache/Cache.java | 37 ++++++++----------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/cache/Cache.java b/server/src/main/java/org/elasticsearch/common/cache/Cache.java index e72f00be6e147..6c3938395d45a 100644 --- a/server/src/main/java/org/elasticsearch/common/cache/Cache.java +++ b/server/src/main/java/org/elasticsearch/common/cache/Cache.java @@ -467,7 +467,7 @@ private void put(K key, V value, long now) { replaced = true; } } - promoteNoLock(tuple.v1(), now); + promote(tuple.v1(), now); } if (replaced) { removalListener.onRemoval(new RemovalNotification<>(tuple.v2().key, tuple.v2().value, @@ -734,27 +734,22 @@ public long getEvictions() { } private void promote(Entry entry, long now) { - try (ReleasableLock ignored = lruLock.acquire()) { - promoteNoLock(entry, now); - } - } - - private void promoteNoLock(Entry entry, long now) { - assert lruLock.isHeldByCurrentThread(); boolean promoted = true; - switch (entry.state) { - case DELETED: - promoted = false; - break; - case EXISTING: - relinkAtHead(entry); - break; - case NEW: - linkAtHead(entry); - break; - } - if (promoted) { - evict(now); + try (ReleasableLock ignored = lruLock.acquire()) { + switch (entry.state) { + case DELETED: + promoted = false; + break; + case EXISTING: + relinkAtHead(entry); + break; + case NEW: + linkAtHead(entry); + break; + } + if (promoted) { + evict(now); + } } }