Replace Guava cache with simple concurrent LRU cache #13879

jasontedor · 2015-09-30T20:14:14Z

This pull request replaces the Guava cache with a simple concurrent LRU cache with flexible eviction policies.

This commit adds a concurrent cache with flexible eviction policies. In particular, this cache supports: 1. concurrency 2. weight-based evictions 3. time-based evictions 4. manual invalidation 5. removal notification 6. cache statistics Closes #13717

This commit removes and now forbids all uses of com.google.common.cache.Cache, com.google.common.cache.CacheBuilder, com.google.common.cache.RemovalListener, com.google.common.cache.RemovalNotification, com.google.common.cache.Weigher across the codebase. This is a major step in the eventual removal of Guava as a dependency. Relates #13224

jpountz · 2015-09-30T20:27:00Z

core/src/main/java/org/elasticsearch/common/cache/Cache.java

+    private long maximumWeight = -1;
+
+    // the weigher of entries
+    private ToLongBiFunction<K, V> weigher = (k, v) -> 1;


Can it be a required argument of the constructor?

@jpountz It can be, but I'm missing the advantage. Can you help me understand why?

I was just trying to see how we could make some variables final so that the compiler ca help us. In addition this one variable looked to me like you would almost always want o use a custom weigher.

I do not think this will have any noticeable impact on the optimizations that the compiler can find; I think this general discussion around final variables should come down to whether or not we want to have a single constructor (still package private) for setting all these optional fields, or use the setters. These fields are semantically final, it's just not being enforced currently because of the approach to use setters to build the cache from the builder.

This commit adds supports for expiration after writes to Cache. This enables entries to expire after they were initially placed in the cache without prolonging their life on retrieval. Replacements are considered new writes.

rmuir · 2015-09-30T22:42:27Z

core/src/test/java/org/elasticsearch/common/cache/CacheTests.java

+                    Integer key = randomIntBetween(1, numberOfEntries);
+                    cache.put(key, randomAsciiOfLength(10));
+                    count.incrementAndGet();
+                    if (rarely()) {


while i know some of these helper methods are friendly, when testing concurrency for something like this, that will be used everywhere, I would remove any other concurrency from the equation: otherwise you have happens-befores that could hide bugs.

All of the lines above cause synchronization: calls to random(), either explicitly or implicitly via randomIntBetween, randomAsciiOfLength, rarely etc synchronize on a global lock in RandomizedContext.
The AtomicInteger seems unnecessary, can we just change this assert at the end to compare against numberOfEntries * numberOfThreads ?

This is a very astute observation, thank you. I've addressed in 0999b7b7bf44a5889178898f32600e784046adbd.

nik9000 · 2015-10-06T20:32:51Z

core/src/main/java/org/elasticsearch/common/cache/Cache.java

+ * evictions are exposed.
+ * <p>
+ * The design of the cache is relatively simple. The cache is segmented into 256 segments which are backed by HashMaps.
+ * The segments are protected by a re-entrant read/write lock. The read/write locks permit multiple concurrent readers


s/The segments are/Each segment is/ ?

Good catch. Addressed in 0fb9081.

jpountz · 2015-10-08T21:21:02Z

core/src/main/java/org/elasticsearch/common/cache/Cache.java

+        V value = get(key, now);
+        if (value == null) {
+            CacheSegment<K, V> segment = getCacheSegment(key);
+            try (ReleasableLock ignored = segment.writeLock.acquire()) {


this lock is only used to ensure that we don't compute the same entry twice at the same time, so I don't think that we actually need to use the segment write lock, which has the downside to block reads on this segment. Should we use a second set of locks?

@jpountz Imagine simultaneous calls to put and computeIfAbsent for the same key. It seems to me the best synchronization mechanism between these two is the segment lock. Note that it's okay if the put overwrites the result of the computeIfAbsent, but we don't want computeIfAbsent to not observe that put is placing a value for the same key (lest we needlessly invoke the loader). Do you still think we should use a different synchronization mechanism?

ok let's avoid the concurrent put/computeIfAbsent issue for now, we can try to improve in the future if we observe slow concurrent access

so maybe let's just leave a comment about what you said?

Sure. Done in a6abb4f.

jpountz · 2015-10-08T21:46:23Z

core/src/main/java/org/elasticsearch/common/cache/Cache.java

+            final Entry<K, V> before = entry.before;
+            final Entry<K, V> after = entry.after;
+
+            if (before == null) {


can we add an assert entry == head for sanity?

Got it in a556e31.

jpountz · 2015-10-08T21:58:44Z

it looks good to me but I would like someone else (@nik9000 ?) to also do another round of review before we merge

jasontedor · 2015-10-09T14:21:15Z

@nik9000 Would you be able to take another look?

nik9000 · 2015-10-09T14:40:39Z

@nik9000 Would you be able to take another look?

Sure! I'll have a look soon.

nik9000 · 2015-10-09T15:03:00Z

core/src/main/java/org/elasticsearch/common/cache/Cache.java

+
+    private boolean promote(Entry<K, V> entry, long now) {
+        boolean promoted = true;
+        try (ReleasableLock ignored = lruLock.acquire()) {


I don't think you need to acquire the lock at all for DELETED, right?

@nik9000 Without this lock it could be the case that entry.state is not State.DELETED, then we enter the lock, and now entry.state is State.DELETED. The check needs to happen after a synchronization barrier prevents any mutations to the entry.

Got it. Maybe a comment for that?

nik9000 · 2015-10-09T15:16:43Z

core/src/main/java/org/elasticsearch/common/cache/Cache.java

+        };
+    }
+
+    private class CacheIterator implements Iterator<Entry<K, V>> {


I'm a bit rusty on my concurrency stuff, but I think there are cases where this will iterate over the same keys twice sometimes or skip some keys. Like if next gets thrown back to the head of the LRU then you start iteration over. Or if a key gets pushed in front of then you skip it. I think we're ok with these, or, rather, we have to be ok with these but its worth documenting if they are indeed possible.

Iteration over the keys was never intended to be thread-safe but only best effort. I agree that we should certainly document this, and I don't think an effort should be made to make it thread-safe.

Added comment in 59c9049.

nik9000 · 2015-10-09T15:28:38Z

Anything else I do will be knit picking. LGTM. Lets get it in and kick the tires.

I think its worth adding a test for the hit and miss stats. I scanned for one and didn't see it but I could have just missed it.

Is it worth adding a stat for additions?

jasontedor · 2015-10-09T15:34:56Z

I think its worth adding a test for the hit and miss stats. I scanned for one and didn't see it but I could have just missed it.

@nik9000 Did you have something in mind that is different than what is in CacheTests#testCacheStats? I think this tests both hit and miss stats, plus evictions.

Is it worth adding a stat for additions?

I don't think so (am I being silly in thinking that it's just Cache.count() + Cache.stats().getEvictions()?

nik9000 · 2015-10-09T15:37:53Z

You are right on both counts there.

LGTM

jasontedor · 2015-10-09T15:45:37Z

@nik9000 @jpountz @dakrone @rmuir Thanks for reviewing. Bringing it home!

Replace Guava cache with simple concurrent LRU cache

jasontedor added 2 commits September 30, 2015 21:43

jasontedor added >enhancement review v5.0.0-alpha1 labels Sep 30, 2015

jpountz reviewed Sep 30, 2015
View reviewed changes

jasontedor added 5 commits September 30, 2015 22:44

Add support for expiration after write to Cache

64727b7

This commit adds supports for expiration after writes to Cache. This enables entries to expire after they were initially placed in the cache without prolonging their life on retrieval. Replacements are considered new writes.

Remove unnecessary overrides of equals/hashCode in Cache.Entry

8c05d4f

Use try-with-resources for lock acquisition

eb2ea01

Ensure that computeIfAbsent loader is invoked at-most once

11d7522

Cleanup formatting

30bfea7

rmuir reviewed Sep 30, 2015
View reviewed changes

jasontedor added 7 commits October 1, 2015 00:47

Lookups from computeIfAbsent should propogate now

7a5e90f

Remove hidden synchronization from test

716be91

Replace CyclicBarrier with CountDownLatch

4efe7b9

Forbidden means verboten!

50cfe71

Start test threads at the same time

c100d18

Cache#computeIfAbsent loader can throw checked exceptions

01e7378

Correct condition for when time is needed

105d830

jasontedor mentioned this pull request Oct 2, 2015

Remove Guava as a dependency #13224

Closed

72 tasks

Add field for expiration conditions

bde4889

nik9000 reviewed Oct 6, 2015
View reviewed changes

jasontedor added 3 commits October 8, 2015 12:04

Correct semantics when loading absent values

02a7d9a

Cache#computeIfAbsent should throw if loader returns null value

8d33be8

Enforce strict eviction semantics

9ca032a

jpountz reviewed Oct 8, 2015
View reviewed changes

jasontedor added 2 commits October 8, 2015 17:43

Use longs for Cache.CacheStats to avoid overflow

e0fa329

Preserve copy of head after taking lock on LRU list

818d217

jpountz reviewed Oct 8, 2015
View reviewed changes

jasontedor added 2 commits October 8, 2015 17:51

Safety assertions on head and tail modifications

a556e31

Safe locking and unlocking of segments during invalidation

881593e

Comment regarding synchronization in Cache#computeIfAbsent

a6abb4f

nik9000 reviewed Oct 9, 2015
View reviewed changes

Release locks in reverse order of acquisition

0149517

nik9000 reviewed Oct 9, 2015
View reviewed changes

Mutating the cache while iterating is undefined

59c9049

jasontedor added a commit that referenced this pull request Oct 9, 2015

Merge pull request #13879 from jasontedor/straight-cache-homey

50368b3

Replace Guava cache with simple concurrent LRU cache

jasontedor merged commit 50368b3 into elastic:master Oct 9, 2015

jasontedor deleted the straight-cache-homey branch October 9, 2015 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Guava cache with simple concurrent LRU cache #13879

Replace Guava cache with simple concurrent LRU cache #13879

jasontedor commented Sep 30, 2015

jpountz Sep 30, 2015

jasontedor Oct 7, 2015

jpountz Oct 7, 2015

jasontedor Oct 8, 2015

rmuir Sep 30, 2015

jasontedor Sep 30, 2015

nik9000 Oct 6, 2015

jasontedor Oct 7, 2015

jpountz Oct 8, 2015

jasontedor Oct 8, 2015

jpountz Oct 8, 2015

jpountz Oct 8, 2015

jasontedor Oct 8, 2015

jpountz Oct 8, 2015

jasontedor Oct 8, 2015

jpountz commented Oct 8, 2015

jasontedor commented Oct 9, 2015

nik9000 commented Oct 9, 2015

nik9000 Oct 9, 2015

jasontedor Oct 9, 2015

nik9000 Oct 9, 2015

nik9000 Oct 9, 2015

jasontedor Oct 9, 2015

nik9000 Oct 9, 2015

jasontedor Oct 9, 2015

nik9000 commented Oct 9, 2015

jasontedor commented Oct 9, 2015

nik9000 commented Oct 9, 2015

jasontedor commented Oct 9, 2015

Replace Guava cache with simple concurrent LRU cache #13879

Replace Guava cache with simple concurrent LRU cache #13879

Conversation

jasontedor commented Sep 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpountz commented Oct 8, 2015

jasontedor commented Oct 9, 2015

nik9000 commented Oct 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Oct 9, 2015

jasontedor commented Oct 9, 2015

nik9000 commented Oct 9, 2015

jasontedor commented Oct 9, 2015