[GitHub] [solr] markrmiller commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery

[GitBox <gi...@apache.org>]

markrmiller commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r676398512



##########
File path: solr/core/src/java/org/apache/solr/search/facet/UnInvertedField.java
##########
@@ -605,80 +605,7 @@ public static UnInvertedField getUnInvertedField(String field, SolrIndexSearcher
     if (cache == null) {
       return new UnInvertedField(field, searcher);
     }
-    AtomicReference<Throwable> throwableRef = new AtomicReference<>();
-    UnInvertedField uif = cache.computeIfAbsent(field, f -> {
-      UnInvertedField newUif;
-      try {
-        newUif = new UnInvertedField(field, searcher);
-      } catch (Throwable t) {
-        throwableRef.set(t);
-        newUif = null;
-      }
-      return newUif;
-    });
-    if (throwableRef.get() != null) {
-      rethrowAsSolrException(field, throwableRef.get());
-    }
-    return uif;
-
-    // (ab) if my understanding is correct this whole block tried to mimic the

Review comment:
       It was not likely trying to mimic ConcurrentHashMap#computeIfAbsent, as it is detrimental to the concurrency of the map due to it's locking. Lot's of other threads can be accessing the map, typically, also using computeIfAbsent. That is why the docs say to make value creation short and sweet. Uninverting fields doesn't really fit that definition. The old impl attempted to use a lock for every single key, the place holder, so that the thread(s) creating a field cache entry did not hamper unrelated map access the entire length of value creation. People have run into ridiculously bad contention with this with ConcurrentHashMap. The previous approach could be taken because the size of the map is essentially tiny given it's key/values are large data structures based on fields. ConcurrentHashMap was designed with requirements that it scale to very large sizes. Once Caffeine became the cache impl, it's computeIfAbsent impl has a mitigation that was added that is generally suitable fo
 r caching - do a get first and only computeIfAbsent if that returns null.




-- 
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