Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove LinkedDeque and replace with LinkedHashMap #6968

Merged
merged 2 commits into from
Apr 5, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -45,15 +47,15 @@ class LRUCache<K, V> implements RefCountedCache<K, V> {
private final HashMap<K, Node<K, V>> data;

/** the LRU list */
private final LinkedDeque<Node<K, V>> lru;
private final LinkedHashMap<K, Node<K, V>> lru;

private final RemovalListener<K, V> listener;

private final Weigher<V> weigher;

private final StatsCounter<K> statsCounter;

private volatile ReentrantLock lock;
private final ReentrantLock lock;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 we could replace

        final ReentrantLock lock = this.lock;
        lock.lock();

with just

        lock.lock();

now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good catch! I'm sure there was some previous incarnation of this code where a volatile lock was needed, but it definitely was never necessary here.


/**
* this tracks cache usage on the system (as long as cache entry is in the cache)
Expand All @@ -65,59 +67,33 @@ class LRUCache<K, V> implements RefCountedCache<K, V> {
*/
private long activeUsage;

static class Node<K, V> implements Linked<Node<K, V>> {
static class Node<K, V> {
final K key;

V value;

long weight;

Node<K, V> prev;

Node<K, V> 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<K, V> getPrevious() {
return prev;
}

public void setPrevious(Node<K, V> prev) {
this.prev = prev;
}

public Node<K, V> getNext() {
return next;
}

public void setNext(Node<K, V> next) {
this.next = next;
}

public boolean evictable() {
return (refCount == 0);
}

V getValue() {
return value;
}
}

public LRUCache(long capacity, RemovalListener<K, V> listener, Weigher<V> weigher) {
this.capacity = capacity;
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<>();

Expand Down Expand Up @@ -250,7 +226,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++;
Expand All @@ -273,7 +249,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) {
Expand All @@ -292,19 +268,15 @@ public long prune() {
final ReentrantLock lock = this.lock;
lock.lock();
try {
Node<K, V> 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<Node<K, V>> iterator = lru.values().iterator();
while (iterator.hasNext()) {
final Node<K, V> 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<K, V> tmp = node;
node = node.getNext();
lru.remove(tmp);
}

usage -= sum;
} finally {
lock.unlock();
Expand Down Expand Up @@ -372,7 +344,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));
Expand All @@ -386,13 +358,10 @@ private boolean hasOverflowed() {
private void evict() {
// Attempts to evict entries from the cache if it exceeds the maximum
// capacity.
while (hasOverflowed()) {
final Node<K, V> node = lru.poll();

if (node == null) {
return;
}

final Iterator<Node<K, V>> iterator = lru.values().iterator();
while (hasOverflowed() && iterator.hasNext()) {
final Node<K, V> node = iterator.next();
iterator.remove();
// Notify the listener only if the entry was evicted
data.remove(node.key, node);
usage -= node.weight;
Expand Down

This file was deleted.

Loading