Performance improvements to use RWLock to access LRUQueryCache (#13306)

Elasticsearch (which based on lucene) can automatically infer types for users with its dynamic mapping feature. When users index some low cardinality fields, such as gender / age / status... they often use some numbers to represent the values, while ES will infer these fields as long, and ES uses BKD as the index of long fields. 

Just as #541 said, when the data volume grows, building the result set of low-cardinality fields will make the CPU usage and load very high even if we use a boolean query with filter clauses for low-cardinality fields. 

One reason is that it uses a ReentrantLock to limit accessing LRUQueryCache. QPS and costs of their queries are often high,  which often causes trying locking failures when obtaining the cache, resulting in low concurrency in accessing the cache.

So I replace the ReentrantLock with a ReentrantReadWriteLock. I only use the read lock when I need to get the cache for a query,

Author: boicehuang

lucene/CHANGES.txt

@@ -292,6 +292,8 @@ Improvements
 Optimizations
 ---------------------
 
+* GITHUB#13306: Use RWLock to access LRUQueryCache to reduce contention. (Boice Huang)
+
 * GITHUB#13252: Replace handwritten loops compare with Arrays.compareUnsigned in SegmentTermsEnum. (zhouhui)
 
 * GITHUB#12996: Reduce ArrayUtil#grow in decompress. (Zhang Chao)

lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java

@@ -32,7 +32,8 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.locks.ReentrantLock;
+import java.util.concurrent.atomic.LongAdder;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 import java.util.function.Predicate;
 import org.apache.lucene.index.IndexReader;
 import org.apache.lucene.index.IndexReaderContext;
@@ -96,14 +97,15 @@ public class LRUQueryCache implements QueryCache, Accountable {
   // mostRecentlyUsedQueries. This is why write operations are performed under a lock
   private final Set<Query> mostRecentlyUsedQueries;
   private final Map<IndexReader.CacheKey, LeafCache> cache;
-  private final ReentrantLock lock;
+  private final ReentrantReadWriteLock.ReadLock readLock;
+  private final ReentrantReadWriteLock.WriteLock writeLock;
   private final float skipCacheFactor;
+  private final LongAdder hitCount;
+  private final LongAdder missCount;
 
   // these variables are volatile so that we do not need to sync reads
   // but increments need to be performed under the lock
   private volatile long ramBytesUsed;
-  private volatile long hitCount;
-  private volatile long missCount;
   private volatile long cacheCount;
   private volatile long cacheSize;
 
@@ -129,11 +131,15 @@ public LRUQueryCache(
     }
     this.skipCacheFactor = skipCacheFactor;
 
-    uniqueQueries = new LinkedHashMap<>(16, 0.75f, true);
+    uniqueQueries = Collections.synchronizedMap(new LinkedHashMap<>(16, 0.75f, true));
     mostRecentlyUsedQueries = uniqueQueries.keySet();
     cache = new IdentityHashMap<>();
-    lock = new ReentrantLock();
+    ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
+    writeLock = lock.writeLock();
+    readLock = lock.readLock();
     ramBytesUsed = 0;
+    hitCount = new LongAdder();
+    missCount = new LongAdder();
   }
 
   /**
@@ -177,8 +183,7 @@ public boolean test(LeafReaderContext context) {
    * @lucene.experimental
    */
   protected void onHit(Object readerCoreKey, Query query) {
-    assert lock.isHeldByCurrentThread();
-    hitCount += 1;
+    hitCount.add(1);
   }
 
   /**
@@ -188,9 +193,8 @@ protected void onHit(Object readerCoreKey, Query query) {
    * @lucene.experimental
    */
   protected void onMiss(Object readerCoreKey, Query query) {
-    assert lock.isHeldByCurrentThread();
     assert query != null;
-    missCount += 1;
+    missCount.add(1);
   }
 
   /**
@@ -201,7 +205,7 @@ protected void onMiss(Object readerCoreKey, Query query) {
    * @lucene.experimental
    */
   protected void onQueryCache(Query query, long ramBytesUsed) {
-    assert lock.isHeldByCurrentThread();
+    assert writeLock.isHeldByCurrentThread();
     this.ramBytesUsed += ramBytesUsed;
   }
 
@@ -212,7 +216,7 @@ protected void onQueryCache(Query query, long ramBytesUsed) {
    * @lucene.experimental
    */
   protected void onQueryEviction(Query query, long ramBytesUsed) {
-    assert lock.isHeldByCurrentThread();
+    assert writeLock.isHeldByCurrentThread();
     this.ramBytesUsed -= ramBytesUsed;
   }
 
@@ -224,7 +228,7 @@ protected void onQueryEviction(Query query, long ramBytesUsed) {
    * @lucene.experimental
    */
   protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {
-    assert lock.isHeldByCurrentThread();
+    assert writeLock.isHeldByCurrentThread();
     cacheSize += 1;
     cacheCount += 1;
     this.ramBytesUsed += ramBytesUsed;
@@ -237,7 +241,7 @@ protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {
    * @lucene.experimental
    */
   protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sumRamBytesUsed) {
-    assert lock.isHeldByCurrentThread();
+    assert writeLock.isHeldByCurrentThread();
     this.ramBytesUsed -= sumRamBytesUsed;
     cacheSize -= numEntries;
   }
@@ -248,14 +252,14 @@ protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sum
    * @lucene.experimental
    */
   protected void onClear() {
-    assert lock.isHeldByCurrentThread();
+    assert writeLock.isHeldByCurrentThread();
     ramBytesUsed = 0;
     cacheSize = 0;
   }
 
   /** Whether evictions are required. */
   boolean requiresEviction() {
-    assert lock.isHeldByCurrentThread();
+    assert writeLock.isHeldByCurrentThread();
     final int size = mostRecentlyUsedQueries.size();
     if (size == 0) {
       return false;
@@ -265,7 +269,6 @@ boolean requiresEviction() {
   }
 
   CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
-    assert lock.isHeldByCurrentThread();
     assert key instanceof BoostQuery == false;
     assert key instanceof ConstantScoreQuery == false;
     final IndexReader.CacheKey readerKey = cacheHelper.getKey();
@@ -293,7 +296,7 @@ private void putIfAbsent(Query query, CacheAndCount cached, IndexReader.CacheHel
     assert query instanceof BoostQuery == false;
     assert query instanceof ConstantScoreQuery == false;
     // under a lock to make sure that mostRecentlyUsedQueries and cache remain sync'ed
-    lock.lock();
+    writeLock.lock();
     try {
       Query singleton = uniqueQueries.putIfAbsent(query, query);
       if (singleton == null) {
@@ -314,12 +317,12 @@ private void putIfAbsent(Query query, CacheAndCount cached, IndexReader.CacheHel
       leafCache.putIfAbsent(query, cached);
       evictIfNecessary();
     } finally {
-      lock.unlock();
+      writeLock.unlock();
     }
   }
 
   private void evictIfNecessary() {
-    assert lock.isHeldByCurrentThread();
+    assert writeLock.isHeldByCurrentThread();
     // under a lock to make sure that mostRecentlyUsedQueries and cache keep sync'ed
     if (requiresEviction()) {
 
@@ -347,7 +350,7 @@ private void evictIfNecessary() {
 
   /** Remove all cache entries for the given core cache key. */
   public void clearCoreCacheKey(Object coreKey) {
-    lock.lock();
+    writeLock.lock();
     try {
       final LeafCache leafCache = cache.remove(coreKey);
       if (leafCache != null) {
@@ -361,25 +364,25 @@ public void clearCoreCacheKey(Object coreKey) {
         }
       }
     } finally {
-      lock.unlock();
+      writeLock.unlock();
     }
   }
 
   /** Remove all cache entries for the given query. */
   public void clearQuery(Query query) {
-    lock.lock();
+    writeLock.lock();
     try {
       final Query singleton = uniqueQueries.remove(query);
       if (singleton != null) {
         onEviction(singleton);
       }
     } finally {
-      lock.unlock();
+      writeLock.unlock();
     }
   }
 
   private void onEviction(Query singleton) {
-    assert lock.isHeldByCurrentThread();
+    assert writeLock.isHeldByCurrentThread();
     onQueryEviction(singleton, getRamBytesUsed(singleton));
     for (LeafCache leafCache : cache.values()) {
       leafCache.remove(singleton);
@@ -388,15 +391,15 @@ private void onEviction(Query singleton) {
 
   /** Clear the content of this cache. */
   public void clear() {
-    lock.lock();
+    writeLock.lock();
     try {
       cache.clear();
       // Note that this also clears the uniqueQueries map since mostRecentlyUsedQueries is the
       // uniqueQueries.keySet view:
       mostRecentlyUsedQueries.clear();
       onClear();
     } finally {
-      lock.unlock();
+      writeLock.unlock();
     }
   }
 
@@ -409,7 +412,7 @@ private static long getRamBytesUsed(Query query) {
 
   // pkg-private for testing
   void assertConsistent() {
-    lock.lock();
+    writeLock.lock();
     try {
       if (requiresEviction()) {
         throw new AssertionError(
@@ -455,18 +458,18 @@ void assertConsistent() {
             "cacheSize mismatch : " + getCacheSize() + " != " + recomputedCacheSize);
       }
     } finally {
-      lock.unlock();
+      writeLock.unlock();
     }
   }
 
   // pkg-private for testing
   // return the list of cached queries in LRU order
   List<Query> cachedQueries() {
-    lock.lock();
+    readLock.lock();
     try {
       return new ArrayList<>(mostRecentlyUsedQueries);
     } finally {
-      lock.unlock();
+      readLock.unlock();
     }
   }
 
@@ -486,11 +489,11 @@ public long ramBytesUsed() {
 
   @Override
   public Collection<Accountable> getChildResources() {
-    lock.lock();
+    writeLock.lock();
     try {
       return Accountables.namedAccountables("segment", cache);
     } finally {
-      lock.unlock();
+      writeLock.unlock();
     }
   }
 
@@ -568,7 +571,7 @@ public final long getTotalCount() {
    * @see #getMissCount()
    */
   public final long getHitCount() {
-    return hitCount;
+    return hitCount.sum();
   }
 
   /**
@@ -579,7 +582,7 @@ public final long getHitCount() {
    * @see #getHitCount()
    */
   public final long getMissCount() {
-    return missCount;
+    return missCount.sum();
   }
 
   /**
@@ -633,11 +636,13 @@ private class LeafCache implements Accountable {
     }
 
     private void onDocIdSetCache(long ramBytesUsed) {
+      assert writeLock.isHeldByCurrentThread();
       this.ramBytesUsed += ramBytesUsed;
       LRUQueryCache.this.onDocIdSetCache(key, ramBytesUsed);
     }
 
     private void onDocIdSetEviction(long ramBytesUsed) {
+      assert writeLock.isHeldByCurrentThread();
       this.ramBytesUsed -= ramBytesUsed;
       LRUQueryCache.this.onDocIdSetEviction(key, 1, ramBytesUsed);
     }
@@ -649,6 +654,7 @@ CacheAndCount get(Query query) {
     }
 
     void putIfAbsent(Query query, CacheAndCount cached) {
+      assert writeLock.isHeldByCurrentThread();
       assert query instanceof BoostQuery == false;
       assert query instanceof ConstantScoreQuery == false;
       if (cache.putIfAbsent(query, cached) == null) {
@@ -658,6 +664,7 @@ void putIfAbsent(Query query, CacheAndCount cached) {
     }
 
     void remove(Query query) {
+      assert writeLock.isHeldByCurrentThread();
       assert query instanceof BoostQuery == false;
       assert query instanceof ConstantScoreQuery == false;
       CacheAndCount removed = cache.remove(query);
@@ -744,15 +751,15 @@ public ScorerSupplier scorerSupplier(LeafReaderContext context) throws IOExcepti
       }
 
       // If the lock is already busy, prefer using the uncached version than waiting
-      if (lock.tryLock() == false) {
+      if (readLock.tryLock() == false) {
         return in.scorerSupplier(context);
       }
 
       CacheAndCount cached;
       try {
         cached = get(in.getQuery(), cacheHelper);
       } finally {
-        lock.unlock();
+        readLock.unlock();
       }
 
       if (cached == null) {
@@ -865,15 +872,15 @@ public int count(LeafReaderContext context) throws IOException {
       }
 
       // If the lock is already busy, prefer using the uncached version than waiting
-      if (lock.tryLock() == false) {
+      if (readLock.tryLock() == false) {
         return -1;
       }
 
       CacheAndCount cached;
       try {
         cached = get(in.getQuery(), cacheHelper);
       } finally {
-        lock.unlock();
+        readLock.unlock();
       }
       if (cached == null) {
         // Not cached
@@ -911,15 +918,15 @@ public BulkScorer bulkScorer(LeafReaderContext context) throws IOException {
       }
 
       // If the lock is already busy, prefer using the uncached version than waiting
-      if (lock.tryLock() == false) {
+      if (readLock.tryLock() == false) {
         return in.bulkScorer(context);
       }
 
       CacheAndCount cached;
       try {
         cached = get(in.getQuery(), cacheHelper);
       } finally {
-        lock.unlock();
+        readLock.unlock();
       }
 
       if (cached == null) {