[GitHub] [solr] noblepaul commented on a diff in pull request #1351: SOLR-16654: Add support for node-level caches

["noblepaul (via GitHub)" <gi...@apache.org>]

noblepaul commented on code in PR #1351:
URL: https://github.com/apache/solr/pull/1351#discussion_r1105271740


##########
solr/core/src/java/org/apache/solr/search/ThinCache.java:
##########
@@ -0,0 +1,465 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.search;
+
+import com.github.benmanes.caffeine.cache.RemovalCause;
+import com.github.benmanes.caffeine.cache.RemovalListener;
+import com.google.common.annotations.VisibleForTesting;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.lang.ref.WeakReference;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.LongAdder;
+import org.apache.lucene.util.Accountable;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.metrics.MetricsMap;
+import org.apache.solr.metrics.SolrMetricsContext;
+import org.apache.solr.util.IOFunction;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A "thin" cache that does not hold strong direct references to the values that it stores and
+ * supplies. Strong references to values are held by a backing {@link NodeLevelCache}. Local
+ * references to keys (and weak references to values) are held by this ThinCache only as an
+ * approximation of the contents of the cache.
+ *
+ * <p>There are no strong guarantees regarding the consistency of local bookkeeping in the ThinCache
+ * (wrt the "source of truth" backing cache). Such guarantees are not necessary, because the local
+ * bookkeeping only exists to support functionality (such as auto-warming and metrics reporting)
+ * where strict correctness is not essential.
+ *
+ * <p>There <i>is</i> however a guarantee that any inconsistency will only be in a safe direction --
+ * i.e., that although there may be entries in the backing cache that are not represented locally in
+ * the ThinCache bookkeeping, the reverse is not true (to protect against memory leak resulting from
+ * the accumulation of stale local references with no corresponding entry in the backing cache).
+ *
+ * <p>NOTE REGARDING AUTOWARMING: because both the warming cache and the cache associated with the
+ * active searcher are backed by the same underlying node-level cache, some extra consideration must
+ * be taken in configuring autowarming. Crosstalk between thin caches is an unavoidable consequence
+ * of the node-level cache approach. Indeed, in the sense that "dynamic resource allocation" is a
+ * type of crosstalk, crosstalk could be considered to be the distinguishing <i>feature</i> of the
+ * node-level cache approach! But in order to prevent competition between active searchers and
+ * corresponding warming searchers, it is advisable to autowarm by percentage -- generally &lt;= 50%
+ * -- or set a relatively low autowarm count (wrt the anticipated overall size of the backing
+ * cache).
+ */
+public class ThinCache<S, K, V> extends SolrCacheBase
+    implements SolrCache<K, V>, Accountable, RemovalListener<K, V> {
+  private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+  private static final class ScopedKey<S, K> {
+    public final S scope;
+    public final K key;
+
+    private ScopedKey(S scope, K key) {
+      this.scope = scope;
+      this.key = key;
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (this == o) return true;
+      if (o == null || getClass() != o.getClass()) return false;
+      ScopedKey<?, ?> scopedKey = (ScopedKey<?, ?>) o;
+      return scope.equals(scopedKey.scope) && key.equals(scopedKey.key);
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hash(scope, key);
+    }
+  }
+
+  private interface RemovalListenerRegistry<S, K, V> extends RemovalListener<ScopedKey<S, K>, V> {
+
+    void register(S scope, RemovalListener<K, V> listener);
+
+    void unregister(S scope);
+  }
+
+  private static class RemovalListenerRegistryImpl<S, K, V>
+      implements RemovalListenerRegistry<S, K, V> {
+
+    private final Map<S, RemovalListener<K, V>> listeners = new ConcurrentHashMap<>();
+
+    @Override
+    public void register(S scope, RemovalListener<K, V> listener) {
+      if (listeners.put(scope, listener) != null) {
+        throw new IllegalStateException("removal listener already added for scope " + scope);
+      }
+    }
+
+    @Override
+    public void unregister(S scope) {
+      if (listeners.remove(scope) == null) {
+        log.warn("no removal listener found for scope {}", scope);
+      }
+    }
+
+    @Override
+    public void onRemoval(ScopedKey<S, K> key, V value, RemovalCause cause) {
+      RemovalListener<K, V> listener;
+      if (key != null && (listener = listeners.get(key.scope)) != null) {
+        listener.onRemoval(key.key, value, cause);
+      }
+    }
+  }
+
+  public static final class NodeLevelCache<S, K, V> extends CaffeineCache<ScopedKey<S, K>, V>
+      implements RemovalListenerRegistry<S, K, V> {
+
+    private final RemovalListenerRegistry<S, K, V> removalListenerRegistry =
+        new RemovalListenerRegistryImpl<>();
+
+    @Override
+    public void onRemoval(ScopedKey<S, K> key, V value, RemovalCause cause) {
+      super.onRemoval(key, value, cause);
+      removalListenerRegistry.onRemoval(key, value, cause);
+    }
+
+    @Override
+    public void register(S scope, RemovalListener<K, V> listener) {
+      removalListenerRegistry.register(scope, listener);
+    }
+
+    @Override
+    public void unregister(S scope) {
+      removalListenerRegistry.unregister(scope);
+    }
+
+    @Override
+    public String getName() {
+      return NodeLevelCache.class.getName();
+    }
+
+    @Override
+    public String getDescription() {
+      return String.format(Locale.ROOT, "Node Level Cache(impl=%s)", super.getDescription());
+    }
+  }
+
+  private String description = "Thin Cache";
+
+  private NodeLevelCache<S, K, V> backing;
+
+  private S scope;
+
+  private String nodeLevelCacheName;
+
+  private final ConcurrentMap<K, ValEntry<V>> local = new ConcurrentHashMap<>();
+
+  private static final class ValEntry<V> {
+    private final LongAdder ct = new LongAdder();
+    private final WeakReference<V> ref;
+
+    private ValEntry(V val) {
+      this.ref = new WeakReference<>(val);
+    }
+  }
+
+  private static final class HitCountEntry<K, V> {
+    private final long ct;
+    private final K key;
+    private final V val;
+
+    private HitCountEntry(long ct, K key, V val) {
+      this.ct = ct;
+      this.key = key;
+      this.val = val;
+    }
+  }
+
+  @VisibleForTesting
+  void setBacking(S scope, NodeLevelCache<S, K, V> backing) {
+    this.scope = scope;
+    this.backing = backing;
+  }
+
+  @Override
+  public void initialSearcher(SolrIndexSearcher initialSearcher) {
+    @SuppressWarnings("unchecked")
+    S scope = (S) initialSearcher.getTopReaderContext().reader().getReaderCacheHelper().getKey();
+    this.scope = scope;
+    @SuppressWarnings("unchecked")
+    NodeLevelCache<S, K, V> backing =
+        (NodeLevelCache<S, K, V>)
+            initialSearcher.getCore().getCoreContainer().getCache(nodeLevelCacheName);
+    this.backing = backing;
+    description = generateDescription();
+    backing.register(scope, this);
+  }
+
+  @Override
+  public void warm(SolrIndexSearcher searcher, SolrCache<K, V> old) {
+    if (searcher != null) {
+      @SuppressWarnings("unchecked")
+      S scope = (S) searcher.getTopReaderContext().reader().getReaderCacheHelper().getKey();
+      this.scope = scope;
+    }
+    backing.register(scope, this);
+    @SuppressWarnings("unchecked")
+    ThinCache<S, K, V> other = (ThinCache<S, K, V>) old;
+    long warmingStartTime = System.nanoTime();
+    List<HitCountEntry<K, V>> orderedEntries = Collections.emptyList();
+    // warm entries
+    if (isAutowarmingOn()) {
+      orderedEntries = new ArrayList<>(other.local.size() << 1); // oversize
+      for (Entry<K, ValEntry<V>> e : other.local.entrySet()) {
+        ValEntry<V> valEntry = e.getValue();
+        V val = valEntry.ref.get();
+        if (val != null) {
+          orderedEntries.add(new HitCountEntry<>(valEntry.ct.sum(), e.getKey(), val));
+        }
+      }
+      orderedEntries.sort((a, b) -> Long.compare(b.ct, a.ct));
+    }
+
+    int size = autowarm.getWarmCount(orderedEntries.size());
+    int ct = 0;
+    for (HitCountEntry<K, V> entry : orderedEntries) {
+      try {
+        boolean continueRegen =
+            regenerator.regenerateItem(searcher, this, old, entry.key, entry.val);
+        if (!continueRegen || ++ct >= size) {
+          break;
+        }
+      } catch (Exception e) {
+        SolrException.log(log, "Error during auto-warming of key:" + entry.key, e);
+      }
+    }
+
+    backing.adjustMetrics(hits.sumThenReset(), inserts.sumThenReset(), lookups.sumThenReset());
+    evictions.reset();
+    priorHits = other.hits.sum() + other.priorHits;
+    priorInserts = other.inserts.sum() + other.priorInserts;
+    priorLookups = other.lookups.sum() + other.priorLookups;
+    priorEvictions = other.evictions.sum() + other.priorEvictions;
+    warmupTime =
+        TimeUnit.MILLISECONDS.convert(System.nanoTime() - warmingStartTime, TimeUnit.NANOSECONDS);
+  }
+
+  @Override
+  public Object init(Map<String, String> args, Object persistence, CacheRegenerator regenerator) {
+    super.init(args, regenerator);
+    nodeLevelCacheName = args.remove("nodeLevelCacheName");

Review Comment:
   is `globalCache` a better name ?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org