From 3efda18e01f085630dba87d4dedbe1ca82b722e6 Mon Sep 17 00:00:00 2001 From: Andrew Ross Date: Tue, 4 Apr 2023 17:03:22 -0700 Subject: [PATCH] Remove LinkedDeque and replace with LinkedHashMap (#6968) * Remove LinkedDeque and replace with LinkedHashMap After the recent changes the usage of the LinkedDeque fits quite well with the insertion order semantics of LinkedHashMap, which also allows for constant time additions and removals. Signed-off-by: Andrew Ross * Use class member reference now that lock is final Signed-off-by: Andrew Ross --------- Signed-off-by: Andrew Ross (cherry picked from commit 65443adb02af0993abe1e1831995b00706ad88c3) --- .../store/remote/utils/cache/LRUCache.java | 75 +-- .../store/remote/utils/cache/Linked.java | 41 -- .../store/remote/utils/cache/LinkedDeque.java | 432 ------------------ 3 files changed, 17 insertions(+), 531 deletions(-) delete mode 100644 server/src/main/java/org/opensearch/index/store/remote/utils/cache/Linked.java delete mode 100644 server/src/main/java/org/opensearch/index/store/remote/utils/cache/LinkedDeque.java diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/cache/LRUCache.java b/server/src/main/java/org/opensearch/index/store/remote/utils/cache/LRUCache.java index 75b28baafe57f..f36055e5d7327 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/utils/cache/LRUCache.java +++ b/server/src/main/java/org/opensearch/index/store/remote/utils/cache/LRUCache.java @@ -17,6 +17,8 @@ import org.opensearch.index.store.remote.utils.cache.stats.StatsCounter; import java.util.HashMap; +import java.util.Iterator; +import java.util.LinkedHashMap; import java.util.Objects; import java.util.concurrent.locks.ReentrantLock; import java.util.function.BiFunction; @@ -45,7 +47,7 @@ class LRUCache implements RefCountedCache { private final HashMap> data; /** the LRU list */ - private final LinkedDeque> lru; + private final LinkedHashMap> lru; private final RemovalListener listener; @@ -53,7 +55,7 @@ class LRUCache implements RefCountedCache { private final StatsCounter statsCounter; - private volatile ReentrantLock lock; + private final ReentrantLock lock; /** * this tracks cache usage on the system (as long as cache entry is in the cache) @@ -65,51 +67,25 @@ class LRUCache implements RefCountedCache { */ private long activeUsage; - static class Node implements Linked> { + static class Node { final K key; V value; long weight; - Node prev; - - Node next; - int refCount; Node(K key, V value, long weight) { this.key = key; this.value = value; this.weight = weight; - this.prev = null; - this.next = null; this.refCount = 0; } - public Node getPrevious() { - return prev; - } - - public void setPrevious(Node prev) { - this.prev = prev; - } - - public Node getNext() { - return next; - } - - public void setNext(Node next) { - this.next = next; - } - public boolean evictable() { return (refCount == 0); } - - V getValue() { - return value; - } } public LRUCache(long capacity, RemovalListener listener, Weigher weigher) { @@ -117,7 +93,7 @@ public LRUCache(long capacity, RemovalListener listener, Weigher weighe this.listener = listener; this.weigher = weigher; this.data = new HashMap<>(); - this.lru = new LinkedDeque<>(); + this.lru = new LinkedHashMap<>(); this.lock = new ReentrantLock(); this.statsCounter = new DefaultStatsCounter<>(); @@ -126,7 +102,6 @@ public LRUCache(long capacity, RemovalListener listener, Weigher weighe @Override public V get(K key) { Objects.requireNonNull(key); - final ReentrantLock lock = this.lock; lock.lock(); try { Node node = data.get(key); @@ -149,7 +124,6 @@ public V put(K key, V value) { Objects.requireNonNull(key); Objects.requireNonNull(value); - final ReentrantLock lock = this.lock; lock.lock(); try { Node node = data.get(key); @@ -170,7 +144,6 @@ public V put(K key, V value) { public V compute(K key, BiFunction remappingFunction) { Objects.requireNonNull(key); Objects.requireNonNull(remappingFunction); - final ReentrantLock lock = this.lock; lock.lock(); try { final Node node = data.get(key); @@ -203,7 +176,6 @@ public V compute(K key, BiFunction remappingF @Override public void remove(K key) { Objects.requireNonNull(key); - final ReentrantLock lock = this.lock; lock.lock(); try { removeNode(key); @@ -214,7 +186,6 @@ public void remove(K key) { @Override public void clear() { - final ReentrantLock lock = this.lock; lock.lock(); try { usage = 0L; @@ -238,7 +209,6 @@ public long size() { @Override public void incRef(K key) { Objects.requireNonNull(key); - final ReentrantLock lock = this.lock; lock.lock(); try { Node node = data.get(key); @@ -250,7 +220,7 @@ public void incRef(K key) { if (node.evictable()) { // since it become active, we should remove it from eviction list - lru.remove(node); + lru.remove(node.key); } node.refCount++; @@ -264,7 +234,6 @@ public void incRef(K key) { @Override public void decRef(K key) { Objects.requireNonNull(key); - final ReentrantLock lock = this.lock; lock.lock(); try { Node node = data.get(key); @@ -273,7 +242,7 @@ public void decRef(K key) { if (node.evictable()) { // if it becomes evictable, we should add it to eviction list - lru.add(node); + lru.put(node.key, node); } if (node.refCount == 0) { @@ -289,22 +258,17 @@ public void decRef(K key) { @Override public long prune() { long sum = 0L; - final ReentrantLock lock = this.lock; lock.lock(); try { - Node node = lru.peek(); - // If weighted values are used, then the pending operations will adjust - // the size to reflect the correct weight - while (node != null) { + final Iterator> iterator = lru.values().iterator(); + while (iterator.hasNext()) { + final Node node = iterator.next(); + iterator.remove(); data.remove(node.key, node); sum += node.weight; statsCounter.recordRemoval(node.weight); listener.onRemoval(new RemovalNotification<>(node.key, node.value, RemovalReason.EXPLICIT)); - Node tmp = node; - node = node.getNext(); - lru.remove(tmp); } - usage -= sum; } finally { lock.unlock(); @@ -314,7 +278,6 @@ public long prune() { @Override public CacheUsage usage() { - final ReentrantLock lock = this.lock; lock.lock(); try { return new CacheUsage(usage, activeUsage); @@ -325,7 +288,6 @@ public CacheUsage usage() { @Override public CacheStats stats() { - final ReentrantLock lock = this.lock; lock.lock(); try { return statsCounter.snapshot(); @@ -372,7 +334,7 @@ private void removeNode(K key) { } usage -= node.weight; if (node.evictable()) { - lru.remove(node); + lru.remove(node.key); } statsCounter.recordRemoval(node.weight); listener.onRemoval(new RemovalNotification<>(node.key, node.value, RemovalReason.EXPLICIT)); @@ -386,13 +348,10 @@ private boolean hasOverflowed() { private void evict() { // Attempts to evict entries from the cache if it exceeds the maximum // capacity. - while (hasOverflowed()) { - final Node node = lru.poll(); - - if (node == null) { - return; - } - + final Iterator> iterator = lru.values().iterator(); + while (hasOverflowed() && iterator.hasNext()) { + final Node node = iterator.next(); + iterator.remove(); // Notify the listener only if the entry was evicted data.remove(node.key, node); usage -= node.weight; diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/cache/Linked.java b/server/src/main/java/org/opensearch/index/store/remote/utils/cache/Linked.java deleted file mode 100644 index 0982909aca874..0000000000000 --- a/server/src/main/java/org/opensearch/index/store/remote/utils/cache/Linked.java +++ /dev/null @@ -1,41 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.index.store.remote.utils.cache; - -import java.util.Deque; - -/** - * An element that is linked on the {@link Deque}. - * - * @opensearch.internal - */ -public interface Linked> { - - /** - * Retrieves the previous element or null if either the element is - * unlinked or the first element on the deque. - */ - T getPrevious(); - - /** - * Sets the previous element or null if there is no link. - */ - void setPrevious(T prev); - - /** - * Retrieves the next element or null if either the element is - * unlinked or the last element on the deque. - */ - T getNext(); - - /** - * Sets the next element or null if there is no link. - */ - void setNext(T next); -} diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/cache/LinkedDeque.java b/server/src/main/java/org/opensearch/index/store/remote/utils/cache/LinkedDeque.java deleted file mode 100644 index 4ca42a7fcf24d..0000000000000 --- a/server/src/main/java/org/opensearch/index/store/remote/utils/cache/LinkedDeque.java +++ /dev/null @@ -1,432 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.index.store.remote.utils.cache; - -import java.util.AbstractCollection; -import java.util.Collection; -import java.util.Deque; -import java.util.Iterator; -import java.util.NoSuchElementException; - -/** - * Linked list implementation of the {@link Deque} interface where the link - * pointers are tightly integrated with the element. Linked deques have no - * capacity restrictions; they grow as necessary to support usage. They are not - * thread-safe; in the absence of external synchronization, they do not support - * concurrent access by multiple threads. Null elements are prohibited. - *

- * Most LinkedDeque operations run in constant time by assuming that - * the {@link Linked} parameter is associated with the deque instance. Any usage - * that violates this assumption will result in non-deterministic behavior. - *

- * The iterators returned by this class are not fail-fast: If - * the deque is modified at any time after the iterator is created, the iterator - * will be in an unknown state. Thus, in the face of concurrent modification, - * the iterator risks arbitrary, non-deterministic behavior at an undetermined - * time in the future. - * - * @param the type of elements held in this collection - * - * @opensearch.internal - */ -public final class LinkedDeque> extends AbstractCollection implements Deque { - // This class provides a doubly-linked list that is optimized for the virtual - // machine. The first and last elements are manipulated instead of a slightly - // more convenient sentinel element to avoid the insertion of null checks with - // NullPointerException throws in the byte code. The links to a removed - // element are cleared to help a generational garbage collector if the - // discarded elements inhabit more than one generation. - - /** - * Pointer to first node. - * Invariant: (first == null && last == null) || - * (first.prev == null) - */ - E first; - - /** - * Pointer to last node. - * Invariant: (first == null && last == null) || - * (last.next == null) - */ - E last; - - /** - * Links the element to the front of the deque so that it becomes the first - * element. - * - * @param e the unlinked element - */ - void linkFirst(final E e) { - final E f = first; - first = e; - - if (f == null) { - last = e; - } else { - f.setPrevious(e); - e.setNext(f); - } - } - - /** - * Links the element to the back of the deque so that it becomes the last - * element. - * - * @param e the unlinked element - */ - void linkLast(final E e) { - final E l = last; - last = e; - - if (l == null) { - first = e; - } else { - l.setNext(e); - e.setPrevious(l); - } - } - - /** - * Unlinks the non-null first element. - */ - E unlinkFirst() { - final E f = first; - final E next = f.getNext(); - f.setNext(null); - - first = next; - if (next == null) { - last = null; - } else { - next.setPrevious(null); - } - return f; - } - - /** - * Unlinks the non-null last element. - */ - E unlinkLast() { - final E l = last; - final E prev = l.getPrevious(); - l.setPrevious(null); - last = prev; - if (prev == null) { - first = null; - } else { - prev.setNext(null); - } - return l; - } - - /** - * Unlinks the non-null element. - */ - void unlink(E e) { - final E prev = e.getPrevious(); - final E next = e.getNext(); - - if (prev == null) { - first = next; - } else { - prev.setNext(next); - e.setPrevious(null); - } - - if (next == null) { - last = prev; - } else { - next.setPrevious(prev); - e.setNext(null); - } - } - - @Override - public boolean isEmpty() { - return (first == null); - } - - void checkNotEmpty() { - if (isEmpty()) { - throw new NoSuchElementException(); - } - } - - /** - * {@inheritDoc} - *

- * Beware that, unlike in most collections, this method is NOT a - * constant-time operation. - */ - @Override - public int size() { - int size = 0; - for (E e = first; e != null; e = e.getNext()) { - size++; - } - return size; - } - - @Override - public void clear() { - for (E e = first; e != null;) { - E next = e.getNext(); - e.setPrevious(null); - e.setNext(null); - e = next; - } - first = last = null; - } - - @Override - public boolean contains(Object o) { - return (o instanceof Linked) && contains((Linked) o); - } - - // A fast-path containment check - boolean contains(Linked e) { - return (e.getPrevious() != null) || (e.getNext() != null) || (e == first); - } - - /** - * Moves the element to the front of the deque so that it becomes the first - * element. - * - * @param e the linked element - */ - public void moveToFront(E e) { - if (e != first) { - unlink(e); - linkFirst(e); - } - } - - /** - * Moves the element to the back of the deque so that it becomes the last - * element. - * - * @param e the linked element - */ - public void moveToBack(E e) { - if (e != last) { - unlink(e); - linkLast(e); - } - } - - @Override - public E peek() { - return peekFirst(); - } - - @Override - public E peekFirst() { - return first; - } - - @Override - public E peekLast() { - return last; - } - - @Override - public E getFirst() { - checkNotEmpty(); - return peekFirst(); - } - - @Override - public E getLast() { - checkNotEmpty(); - return peekLast(); - } - - @Override - public E element() { - return getFirst(); - } - - @Override - public boolean offer(E e) { - return offerLast(e); - } - - @Override - public boolean offerFirst(E e) { - if (contains(e)) { - return false; - } - linkFirst(e); - return true; - } - - @Override - public boolean offerLast(E e) { - if (contains(e)) { - return false; - } - linkLast(e); - return true; - } - - @Override - public boolean add(E e) { - return offerLast(e); - } - - @Override - public void addFirst(E e) { - if (!offerFirst(e)) { - throw new IllegalArgumentException(); - } - } - - @Override - public void addLast(E e) { - if (!offerLast(e)) { - throw new IllegalArgumentException(); - } - } - - @Override - public E poll() { - return pollFirst(); - } - - @Override - public E pollFirst() { - return isEmpty() ? null : unlinkFirst(); - } - - @Override - public E pollLast() { - return isEmpty() ? null : unlinkLast(); - } - - @Override - public E remove() { - return removeFirst(); - } - - @Override - @SuppressWarnings("unchecked") - public boolean remove(Object o) { - return (o instanceof Linked) && remove((E) o); - } - - // A fast-path removal - boolean remove(E e) { - if (contains(e)) { - unlink(e); - return true; - } - return false; - } - - @Override - public E removeFirst() { - checkNotEmpty(); - return pollFirst(); - } - - @Override - public boolean removeFirstOccurrence(Object o) { - return remove(o); - } - - @Override - public E removeLast() { - checkNotEmpty(); - return pollLast(); - } - - @Override - public boolean removeLastOccurrence(Object o) { - return remove(o); - } - - @Override - public boolean removeAll(Collection c) { - boolean modified = false; - for (Object o : c) { - modified |= remove(o); - } - return modified; - } - - @Override - public void push(E e) { - addFirst(e); - } - - @Override - public E pop() { - return removeFirst(); - } - - @Override - public Iterator iterator() { - return new AbstractLinkedIterator(first) { - @Override - E computeNext() { - return cursor.getNext(); - } - }; - } - - @Override - public Iterator descendingIterator() { - return new AbstractLinkedIterator(last) { - @Override - E computeNext() { - return cursor.getPrevious(); - } - }; - } - - abstract class AbstractLinkedIterator implements Iterator { - E cursor; - - /** - * Creates an iterator that can can traverse the deque. - * - * @param start the initial element to begin traversal from - */ - AbstractLinkedIterator(E start) { - cursor = start; - } - - @Override - public boolean hasNext() { - return (cursor != null); - } - - @Override - public E next() { - if (!hasNext()) { - throw new NoSuchElementException(); - } - E e = cursor; - cursor = computeNext(); - return e; - } - - @Override - public void remove() { - throw new UnsupportedOperationException(); - } - - /** - * Retrieves the next element to traverse to or null if there are - * no more elements. - */ - abstract E computeNext(); - } -}