Allow skip cache factor to be updated dynamically

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

@@ -32,6 +32,7 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.concurrent.atomic.LongAdder;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 import java.util.function.Predicate;
@@ -99,7 +100,7 @@ public class LRUQueryCache implements QueryCache, Accountable {
   private final Map<IndexReader.CacheKey, LeafCache> cache;
   private final ReentrantReadWriteLock.ReadLock readLock;
   private final ReentrantReadWriteLock.WriteLock writeLock;
-  private final float skipCacheFactor;
+  private final AtomicReference<Float> skipCacheFactor;
   private final LongAdder hitCount;
   private final LongAdder missCount;
 
@@ -122,12 +123,30 @@ public LRUQueryCache(
       long maxRamBytesUsed,
       Predicate<LeafReaderContext> leavesToCache,
       float skipCacheFactor) {
+    this(maxSize, maxRamBytesUsed, leavesToCache, new AtomicReference<>(skipCacheFactor));
+  }
+
+  /**
+   * Additionally, allows the ability to pass skipCacheFactor in form of AtomicReference where the
+   * caller can dynamically update(in a thread safe way) its value by calling skipCacheFactor.set()
+   * on their end.
+   */
+  public LRUQueryCache(
+      int maxSize,
+      long maxRamBytesUsed,
+      Predicate<LeafReaderContext> leavesToCache,
+      AtomicReference<Float> skipCacheFactor) {
     this.maxSize = maxSize;
     this.maxRamBytesUsed = maxRamBytesUsed;
     this.leavesToCache = leavesToCache;
-    if (skipCacheFactor >= 1 == false) { // NaN >= 1 evaluates false
+    if (skipCacheFactor == null || skipCacheFactor.get() == null) {
+      throw new IllegalArgumentException("skipCacheFactor should not be null");
+    }
+    if (skipCacheFactor.get() < 1) { //
+      // NaN >= 1
+      // evaluates false
       throw new IllegalArgumentException(
-          "skipCacheFactor must be no less than 1, get " + skipCacheFactor);
+          "skipCacheFactor must be no less than 1, get " + skipCacheFactor.get());
     }
     this.skipCacheFactor = skipCacheFactor;
 
@@ -142,6 +161,10 @@ public LRUQueryCache(
     missCount = new LongAdder();
   }
 
+  AtomicReference<Float> getSkipCacheFactor() {
+    return skipCacheFactor;
+  }
+
   /**
    * Create a new instance that will cache at most <code>maxSize</code> queries with at most <code>
    * maxRamBytesUsed</code> bytes of memory. Queries will only be cached on leaves that have more
@@ -269,8 +292,8 @@ boolean requiresEviction() {
   }
 
   CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
-    assert key instanceof BoostQuery == false;
-    assert key instanceof ConstantScoreQuery == false;
+    assert !(key instanceof BoostQuery);
+    assert !(key instanceof ConstantScoreQuery);
     final IndexReader.CacheKey readerKey = cacheHelper.getKey();
     final LeafCache leafCache = cache.get(readerKey);
     if (leafCache == null) {
@@ -293,8 +316,8 @@ CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
   }
 
   private void putIfAbsent(Query query, CacheAndCount cached, IndexReader.CacheHelper cacheHelper) {
-    assert query instanceof BoostQuery == false;
-    assert query instanceof ConstantScoreQuery == false;
+    assert !(query instanceof BoostQuery);
+    assert !(query instanceof ConstantScoreQuery);
     // under a lock to make sure that mostRecentlyUsedQueries and cache remain sync'ed
     writeLock.lock();
     try {
@@ -652,15 +675,15 @@ private void onDocIdSetEviction(long ramBytesUsed) {
     }
 
     CacheAndCount get(Query query) {
-      assert query instanceof BoostQuery == false;
-      assert query instanceof ConstantScoreQuery == false;
+      assert !(query instanceof BoostQuery);
+      assert !(query instanceof ConstantScoreQuery);
       return cache.get(query);
     }
 
     void putIfAbsent(Query query, CacheAndCount cached) {
       assert writeLock.isHeldByCurrentThread();
-      assert query instanceof BoostQuery == false;
-      assert query instanceof ConstantScoreQuery == false;
+      assert !(query instanceof BoostQuery);
+      assert !(query instanceof ConstantScoreQuery);
       if (cache.putIfAbsent(query, cached) == null) {
         // the set was actually put
         onDocIdSetCache(HASHTABLE_RAM_BYTES_PER_ENTRY + cached.ramBytesUsed());
@@ -669,8 +692,8 @@ void putIfAbsent(Query query, CacheAndCount cached) {
 
     void remove(Query query) {
       assert writeLock.isHeldByCurrentThread();
-      assert query instanceof BoostQuery == false;
-      assert query instanceof ConstantScoreQuery == false;
+      assert !(query instanceof BoostQuery);
+      assert !(query instanceof ConstantScoreQuery);
       CacheAndCount removed = cache.remove(query);
       if (removed != null) {
         onDocIdSetEviction(HASHTABLE_RAM_BYTES_PER_ENTRY + removed.ramBytesUsed());
@@ -706,13 +729,12 @@ public Matches matches(LeafReaderContext context, int doc) throws IOException {
     private boolean cacheEntryHasReasonableWorstCaseSize(int maxDoc) {
       // The worst-case (dense) is a bit set which needs one bit per document
       final long worstCaseRamUsage = maxDoc / 8;
-      final long totalRamAvailable = maxRamBytesUsed;
       // Imagine the worst-case that a cache entry is large than the size of
       // the cache: not only will this entry be trashed immediately but it
       // will also evict all current entries from the cache. For this reason
       // we only cache on an IndexReader if we have available room for
       // 5 different filters on this reader to avoid excessive trashing
-      return worstCaseRamUsage * 5 < totalRamAvailable;
+      return worstCaseRamUsage * 5 < maxRamBytesUsed;
     }
 
     /** Check whether this segment is eligible for caching, regardless of the query. */
@@ -728,14 +750,14 @@ public ScorerSupplier scorerSupplier(LeafReaderContext context) throws IOExcepti
         policy.onUse(getQuery());
       }
 
-      if (in.isCacheable(context) == false) {
+      if (!in.isCacheable(context)) {
         // this segment is not suitable for caching
         return in.scorerSupplier(context);
       }
 
       // Short-circuit: Check whether this segment is eligible for caching
       // before we take a lock because of #get
-      if (shouldCache(context) == false) {
+      if (!shouldCache(context)) {
         return in.scorerSupplier(context);
       }
 
@@ -746,7 +768,7 @@ public ScorerSupplier scorerSupplier(LeafReaderContext context) throws IOExcepti
       }
 
       // If the lock is already busy, prefer using the uncached version than waiting
-      if (readLock.tryLock() == false) {
+      if (!readLock.tryLock()) {
         return in.scorerSupplier(context);
       }
 
@@ -771,7 +793,7 @@ public ScorerSupplier scorerSupplier(LeafReaderContext context) throws IOExcepti
             @Override
             public DocIdSetIterator iterator(long leadCost) throws IOException {
               // skip cache operation which would slow query down too much
-              if (cost / skipCacheFactor > leadCost) {
+              if (cost / skipCacheFactor.get() > leadCost) {
                 return supplier.get(leadCost).iterator();
               }
 
@@ -829,7 +851,7 @@ public int count(LeafReaderContext context) throws IOException {
 
       // Short-circuit: Check whether this segment is eligible for caching
       // before we take a lock because of #get
-      if (shouldCache(context) == false) {
+      if (!shouldCache(context)) {
         return in.count(context);
       }
 
@@ -840,7 +862,7 @@ public int count(LeafReaderContext context) throws IOException {
       }
 
       // If the lock is already busy, prefer using the uncached version than waiting
-      if (readLock.tryLock() == false) {
+      if (!readLock.tryLock()) {
         return in.count(context);
       }
 

lucene/core/src/test/org/apache/lucene/search/TestLRUQueryCache.java

@@ -2131,4 +2131,68 @@ public void testCacheHasFastCount() throws IOException {
     w.close();
     dir.close();
   }
+
+  public void testSkipCacheFactorWithDynamicValues() throws IOException {
+    Directory dir = newDirectory();
+    final RandomIndexWriter w = new RandomIndexWriter(random(), dir);
+    Document doc1 = new Document();
+    doc1.add(new StringField("name", "tom", Store.YES));
+    doc1.add(new StringField("hobby", "movie", Store.YES));
+    Document doc2 = new Document();
+    doc2.add(new StringField("name", "alice", Store.YES));
+    doc2.add(new StringField("hobby", "book", Store.YES));
+    Document doc3 = new Document();
+    doc3.add(new StringField("name", "alice", Store.YES));
+    doc3.add(new StringField("hobby", "movie", Store.YES));
+    w.addDocuments(Arrays.asList(doc1, doc2, doc3));
+    final IndexReader reader = w.getReader();
+    final IndexSearcher searcher = newSearcher(reader);
+    final QueryCachingPolicy policy =
+        new QueryCachingPolicy() {
+
+          @Override
+          public boolean shouldCache(Query query) throws IOException {
+            return query.getClass() != TermQuery.class;
+          }
+
+          @Override
+          public void onUse(Query query) {
+            // no-op
+          }
+        };
+    searcher.setQueryCachingPolicy(policy);
+    w.close();
+
+    // lead cost is 2, cost of subQuery1 is 3, cost of subQuery2 is 2
+    BooleanQuery.Builder inner = new BooleanQuery.Builder();
+    TermQuery innerSubQuery1 = new TermQuery(new Term("hobby", "book"));
+    TermQuery innerSubQuery2 = new TermQuery(new Term("hobby", "movie"));
+    BooleanQuery subQuery1 =
+        inner.add(innerSubQuery1, Occur.SHOULD).add(innerSubQuery2, Occur.SHOULD).build();
+
+    BooleanQuery.Builder bq = new BooleanQuery.Builder();
+    TermQuery subQuery2 = new TermQuery(new Term("name", "alice"));
+    BooleanQuery query =
+        bq.add(new ConstantScoreQuery(subQuery1), Occur.FILTER)
+            .add(subQuery2, Occur.FILTER)
+            .build();
+    Set<Query> cacheSet = new HashSet<>();
+
+    // Define skip cache factor to be 1, so that query is not cached.
+    AtomicReference<Float> skipCacheFactor = new AtomicReference<>(1.0f);
+    final LRUQueryCache cache = new LRUQueryCache(1000000, 10000000, _ -> true, skipCacheFactor);
+    searcher.setQueryCache(cache);
+    searcher.search(query, 1);
+    assertEquals(cacheSet, new HashSet<>(cache.cachedQueries()));
+    assertEquals(1.0, cache.getSkipCacheFactor().get().floatValue(), 0);
+
+    // Now change the skipCacheFactor to 3.0f, now same query should get cached.
+    skipCacheFactor.set(3.0f);
+    searcher.search(query, 1);
+    assertEquals(new HashSet<>(List.of(subQuery1)), new HashSet<>(cache.cachedQueries()));
+    assertEquals(3.0, cache.getSkipCacheFactor().get().floatValue(), 0);
+
+    reader.close();
+    dir.close();
+  }
 }