You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/07/21 18:18:15 UTC
[GitHub] [solr] madrob opened a new pull request #230: SOLR-15555 Improved caching on FilterQuery
madrob opened a new pull request #230:
URL: https://github.com/apache/solr/pull/230
https://issues.apache.org/jira/browse/SOLR-15555
--
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
[GitHub] [solr] madrob commented on pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #230:
URL: https://github.com/apache/solr/pull/230#issuecomment-885259624
@sigram can't tag you as a reviewer, but would appreciate you taking a look as well, if you have time. Thanks!
--
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
[GitHub] [solr] markrmiller commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
markrmiller commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r706265995
##########
File path: solr/solr-ref-guide/src/caches-warming.adoc
##########
@@ -79,6 +79,9 @@ Reasonable values, depending on the query volume and patterns, may lie somewhere
The `maxRamMB` attribute limits the maximum amount of memory a cache may consume.
When both `size` and `maxRamMB` limits are specified the `maxRamMB` limit will take precedence and the `size` limit will be ignored.
+The `async` attribute determines whether the cache stores direct results (`async=false`, the default) or whether it will store indirect references to the computation (`async=true`). Enabling `async` will use more slightly more memory per cache entry, but may result in reduced CPU cycles generating results or doing garbage collection. The most noticable improvements will be seen when there are many queries racing to compute the same results before they are inserted into the cache. In some use cases, increasing autowarming may be a better alternative to enabling an async cache. Additionally, an async cache will not prevent a data race for time-limited queries, as those may return differing sets of partial results.
Review comment:
Nothing urgent here, but personally I'd like it to also be a little more clear that this issue can help solve the problem in the description of the JIRA. It's somewhat referenced with the mention of autowarming, but with no history of that issue, I'd have no idea that using this would help prevent that problem or why autowarming was somehow another approach to the async option.
--
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
[GitHub] [solr] ben-manes commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
ben-manes commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r685465632
##########
File path: solr/core/src/java/org/apache/solr/search/CaffeineCache.java
##########
@@ -176,33 +198,95 @@ public V get(K key) {
return cache.getIfPresent(key);
}
- @Override
- public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) {
- return cache.get(key, k -> {
- inserts.increment();
- V value = mappingFunction.apply(k);
- if (value == null) {
- return null;
+ private V computeAsync(K key, IOFunction<? super K, ? extends V> mappingFunction) throws IOException {
+ CompletableFuture<V> future = new CompletableFuture<>();
+ CompletableFuture<V> result = asyncCache.asMap().putIfAbsent(key, future);
+ lookups.increment();
+ if (result != null) {
+ try {
+ // Another thread is already working on this computation, wait for them to finish
+ V value = result.join();
+ hits.increment();
+ return value;
+ } catch (CompletionException e) {
+ Throwable cause = e.getCause();
+ if (cause instanceof IOException) {
+ // Computation had an IOException, likely index problems, so fail this result too
+ throw (IOException) cause;
+ }
+ if (cause instanceof CancellableCollector.QueryCancelledException) {
+ // The reserved slot that we were waiting for got cancelled, so we will compute directly
+ // If we go back to waiting for a new cache result then that can lead to thread starvation
+ // Should we record a cache miss here?
+ return mappingFunction.apply(key);
+ }
+ throw e;
}
- ramBytes.add(RamUsageEstimator.sizeOfObject(key, RamUsageEstimator.QUERY_DEFAULT_RAM_BYTES_USED) +
- RamUsageEstimator.sizeOfObject(value, RamUsageEstimator.QUERY_DEFAULT_RAM_BYTES_USED));
- ramBytes.add(RamUsageEstimator.LINKED_HASHTABLE_RAM_BYTES_PER_ENTRY);
+ }
+ try {
+ // We reserved the slot, so we do the work
+ V value = mappingFunction.apply(key);
+ future.complete(value); // This will update the weight and expiration
+ recordRamBytes(key, null, value);
+ inserts.increment();
return value;
- });
+ } catch (RuntimeException | IOException e) {
+ // TimeExceeded exception is runtime and will bubble up from here
+ future.completeExceptionally(e); // This will remove the future from the cache
+ throw e;
+ }
+ }
+
+ @Override
+ public V computeIfAbsent(K key, IOFunction<? super K, ? extends V> mappingFunction) throws IOException {
+ if (async) {
+ return computeAsync(key, mappingFunction);
+ }
+
+ try {
+ return cache.get(key, k -> {
+ V value;
+ try {
+ value = mappingFunction.apply(k);
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ if (value == null) {
+ return null;
+ }
+ recordRamBytes(key, null, value);
+ inserts.increment();
+ return value;
+ });
+ } catch (UncheckedIOException e) {
+ throw e.getCause();
+ }
}
@Override
public V put(K key, V val) {
inserts.increment();
V old = cache.asMap().put(key, val);
+ cache.put(key, val);
Review comment:
remove redundant put?
--
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
[GitHub] [solr] madrob commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r674457589
##########
File path: solr/core/src/java/org/apache/solr/search/SolrCache.java
##########
@@ -106,8 +107,23 @@
* @return existing or newly computed value, null if there was no existing value and
* it was not possible to compute a new value (in which case the new mapping won't be created).
*/
+ // Can we deprecate/remove this in favor of the other signature?
public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction);
+ default V computeIfAbsent(K key, IOFunction<? super K, ? extends V> mappingFunction) throws IOException {
+ try {
+ return computeIfAbsent(key, (Function<? super K, ? extends V>) k -> {
+ try {
+ return mappingFunction.apply(k);
+ } catch (IOException e) {
+ throw new WrappingException(e);
Review comment:
Can also pass this along via an AtomicReference or possible via Lucene's Rethrows class if people would prefer to see that.
##########
File path: solr/core/src/test/org/apache/solr/search/TestRangeQuery.java
##########
@@ -357,7 +371,42 @@ public void process(SolrInputDocument doc) {
}
}
}
-
+
+ @Test
+ public void testRangeQueryWithFilterCache() throws Exception {
+ TestInjection.delayBeforeCreatingNewDocSet = 500;
+
+ // sometimes a very small index, sometimes a very large index
+ // final int numDocs = random().nextBoolean() ? random().nextInt(50) : atLeast(1000);
+ final int numDocs = 99;
+ createIndex(numDocs, doc -> {
+ addInt(doc, 0, 0, "foo_i");
+ });
+
+ ExecutorService queryService = ExecutorUtil.newMDCAwareFixedThreadPool(4, new SolrNamedThreadFactory("TestRangeQuery"));
+ try (SolrCore core = h.getCoreInc()) {
+ SolrRequestHandler defaultHandler = core.getRequestHandler("");
+
+ ModifiableSolrParams params = new ModifiableSolrParams();
+ params.set("q", "*:*");
+ params.add("fq", "id:[0 TO 222]"); // These should all come from FilterCache
+
+ // 10 threads with 4 executors would be enough for 3 waves, or approximately 1500ms of delay
+ for (int i = 0; i < 10; i++) {
+ queryService.submit(() -> {
+ try (SolrQueryRequest req = req(params)) {
+ core.execute(defaultHandler, req, new SolrQueryResponse());
+ }
+ });
+ }
+
Review comment:
TODO: assert that createDocSet was only called once here instead of 4 times.
--
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
[GitHub] [solr] madrob commented on pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #230:
URL: https://github.com/apache/solr/pull/230#issuecomment-917494002
@markrmiller I did some timing runs of this, and the setup cost per Trial works out to about 20s each. I have 2 benchmarks with 5 * 3 combinations each, so that is already 10 minutes, which is beyond your intuition's maximum, and we haven't even run any tests yet. Doing a smaller data set (1k docs instead of 30k) gets the setup to 13s each trial.
Maybe we only commit the freq=50 benchmark by default and let power users run with 2 and 98 by themselves? That would cut the default time in third.
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r679492857
##########
File path: solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
##########
@@ -804,15 +806,39 @@ public BitDocSet getDocSetBits(Query q) throws IOException {
}
// only handle positive (non negative) queries
- DocSet getPositiveDocSet(Query q) throws IOException {
- DocSet answer;
- if (filterCache != null) {
- answer = filterCache.get(q);
- if (answer != null) return answer;
+ DocSet getPositiveDocSet(Query query) throws IOException {
+ // TODO duplicated code with getDocSet?
+ boolean doCache = filterCache != null;
+ if (query instanceof ExtendedQuery) {
+ if (!((ExtendedQuery) query).getCache()) {
+ doCache = false;
+ }
+ if (query instanceof WrappedQuery) {
+ query = ((WrappedQuery) query).getWrappedQuery();
+ }
}
- answer = getDocSetNC(q, null);
- if (filterCache != null) filterCache.put(q, answer);
- return answer;
+
+ if (doCache) {
+ return getAndCacheDocSet(query);
+ }
+
+ return getDocSetNC(query, null);
+ }
+
+ /**
+ * Attempt to read the query from the filter cache, if not will compute the result and insert back into the cache
+ * <p>Callers must ensure that:
+ * <ul><li>The query is unwrapped
+ * <li>The query has caching enabled
+ * <li>The filter cache exists
+ * @param query the query to compute.
+ * @return the DocSet answer
+ */
+ private DocSet getAndCacheDocSet(Query query) throws IOException {
+ assert !(query instanceof WrappedQuery) : "should have unwrapped";
+ assert filterCache != null : "must check for caching before calling this method";
+
+ return filterCache.computeIfAbsent(query, q -> getDocSetNC(q, null));
Review comment:
Sweet :-)
--
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
[GitHub] [solr] madrob commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r676967652
##########
File path: solr/core/src/java/org/apache/solr/query/SolrRangeQuery.java
##########
@@ -452,6 +455,7 @@ private SegState getSegState(LeafReaderContext context) throws IOException {
if (doCheck) {
answer = createDocSet(solrSearcher, count);
+ // This can be a naked put because the cache usually gets checked in SolrIndexSearcher
Review comment:
A naked put in this context is a put without a previous call to get or without a computeIfAbsent. We typically don't want to just insert things into the cache, but in this case it's _probably_ ok because we would have already checked in SIS if the entry exists.
--
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
[GitHub] [solr] madrob commented on pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #230:
URL: https://github.com/apache/solr/pull/230#issuecomment-918605100
Pushed the benchmark portion of this separately in c942c7242c2, rebased, will wait for precommit to finish one more time before merging.
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r676293197
##########
File path: solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
##########
@@ -804,15 +807,29 @@ public BitDocSet getDocSetBits(Query q) throws IOException {
}
// only handle positive (non negative) queries
- DocSet getPositiveDocSet(Query q) throws IOException {
- DocSet answer;
- if (filterCache != null) {
- answer = filterCache.get(q);
- if (answer != null) return answer;
+ DocSet getPositiveDocSet(Query query) throws IOException {
+ // TODO duplicated code with getDocSet, can maybe also use FutureDocSet here
+ boolean doCache = filterCache != null;
Review comment:
I like this `doCache` local var for clarity but otherwise I don't see why you are making any changes to this method. It also appears getDocSetNC might get called twice if there's a cache miss?
##########
File path: solr/core/src/test/org/apache/solr/request/TestUnInvertedFieldException.java
##########
@@ -96,13 +96,9 @@ public void testConcurrentInit() throws Exception {
List<Future<UnInvertedField>> futures = initCallables.stream().map((c) -> pool.submit(c))
.collect(Collectors.toList());
for (Future<UnInvertedField> uifuture : futures) {
- try {
- final UnInvertedField uif = uifuture.get();
- } catch (ExecutionException injection) {
- SolrException solrException = (SolrException) injection.getCause();
- assertEquals(ErrorCode.SERVER_ERROR.code, solrException.code());
- assertSame(solrException.getCause().getClass(), OutOfMemoryError.class);
- }
+ ExecutionException injection = assertThrows(ExecutionException.class, uifuture::get);
Review comment:
Nice
##########
File path: solr/core/src/java/org/apache/solr/search/SolrCache.java
##########
@@ -106,8 +108,23 @@
* @return existing or newly computed value, null if there was no existing value and
* it was not possible to compute a new value (in which case the new mapping won't be created).
*/
+ // Can we deprecate/remove this in favor of the other signature?
public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction);
+ default V computeIfAbsent(K key, IOFunction<? super K, ? extends V> mappingFunction) throws IOException {
Review comment:
I like this; it simplified some of the callers nicely.
##########
File path: solr/core/src/java/org/apache/solr/search/SolrCache.java
##########
@@ -106,8 +108,23 @@
* @return existing or newly computed value, null if there was no existing value and
* it was not possible to compute a new value (in which case the new mapping won't be created).
*/
+ // Can we deprecate/remove this in favor of the other signature?
Review comment:
Yes
##########
File path: solr/core/src/java/org/apache/solr/query/SolrRangeQuery.java
##########
@@ -452,6 +455,7 @@ private SegState getSegState(LeafReaderContext context) throws IOException {
if (doCheck) {
answer = createDocSet(solrSearcher, count);
+ // This can be a naked put because the cache usually gets checked in SolrIndexSearcher
Review comment:
I don't follow your meaning, nor what a "naked put" is
##########
File path: solr/core/src/java/org/apache/solr/schema/RptWithGeometrySpatialField.java
##########
@@ -163,10 +164,11 @@ public Shape value() throws IOException {
throw new IllegalStateException("Leaf " + readerContext.reader() + " is not suited for caching");
}
PerSegCacheKey key = new PerSegCacheKey(cacheHelper.getKey(), docId);
- Shape shape = cache.computeIfAbsent(key, k -> {
+ Shape shape = cache.computeIfAbsent(key, (Function<PerSegCacheKey, Shape>) k -> {
try {
return targetFuncValues.value();
} catch (IOException e) {
+ // Strange that this returns null instead of propagating exceptions
Review comment:
Lets propagate it. I am guessing maybe I was lazy here :-(
--
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
[GitHub] [solr] markrmiller commented on pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
markrmiller commented on pull request #230:
URL: https://github.com/apache/solr/pull/230#issuecomment-914957498
I'm seeing some pretty bad test results in the latest work here - dozens to a 150 or so fails due to what looks initially like at least two separate NPE cases.
--
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
[GitHub] [solr] ben-manes commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
ben-manes commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r683680979
##########
File path: solr/core/src/java/org/apache/solr/search/CaffeineCache.java
##########
@@ -190,73 +191,72 @@ public V get(K key) {
return cache.getIfPresent(key);
}
- @Override
- public V computeIfAbsent(K key, IOFunction<? super K, ? extends V> mappingFunction) throws IOException {
- /*
- This block will get deleted before commit, but I'm leaving it in for now in case I have to go back to that apporach
+ private V computeAsync(K key, IOFunction<? super K, ? extends V> mappingFunction) throws IOException {
+ // First do a lookup first: optimistic non-locking path.
+ // The cache does this for us on get(K, Function), but not putIfAbsent
+ V value = get(key);
+ if (value != null) {
+ return value;
+ }
Review comment:
Please upgrade to the latest Caffeine version. That oversight was fixed.
--
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
[GitHub] [solr] madrob commented on pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #230:
URL: https://github.com/apache/solr/pull/230#issuecomment-895478529
Rebased and squashed, would appreciate another look-over from folks that have participated. Working on additional performance testing before committing, but otherwise I think this is pretty close.
--
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
[GitHub] [solr] madrob commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r706349788
##########
File path: solr/solr-ref-guide/src/caches-warming.adoc
##########
@@ -79,6 +79,9 @@ Reasonable values, depending on the query volume and patterns, may lie somewhere
The `maxRamMB` attribute limits the maximum amount of memory a cache may consume.
When both `size` and `maxRamMB` limits are specified the `maxRamMB` limit will take precedence and the `size` limit will be ignored.
+The `async` attribute determines whether the cache stores direct results (`async=false`, the default) or whether it will store indirect references to the computation (`async=true`). Enabling `async` will use more slightly more memory per cache entry, but may result in reduced CPU cycles generating results or doing garbage collection. The most noticable improvements will be seen when there are many queries racing to compute the same results before they are inserted into the cache. In some use cases, increasing autowarming may be a better alternative to enabling an async cache. Additionally, an async cache will not prevent a data race for time-limited queries, as those may return differing sets of partial results.
Review comment:
This already has
> The most noticable improvements will be seen when there are many queries racing to compute the same results before they are inserted into the cache.
Not sure what else you mean I should add?
--
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
[GitHub] [solr] markrmiller commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
markrmiller commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r706260246
##########
File path: solr/core/src/java/org/apache/solr/search/CaffeineCache.java
##########
@@ -114,9 +126,12 @@ public Object init(Map<String, String> args, Object persistence, CacheRegenerato
str = args.get(MAX_RAM_MB_PARAM);
int maxRamMB = str == null ? -1 : Double.valueOf(str).intValue();
maxRamBytes = maxRamMB < 0 ? Long.MAX_VALUE : maxRamMB * 1024L * 1024L;
- str = args.get(CLEANUP_THREAD_PARAM);
- cleanupThread = str != null && Boolean.parseBoolean(str);
- if (cleanupThread) {
+ cleanupThread = Boolean.parseBoolean(args.get(CLEANUP_THREAD_PARAM));
+ async = Boolean.parseBoolean(args.get(ASYNC_PARAM));
+ if (async) {
+ // We record futures in the map to decrease bucket-lock contention, but need computation handled in same thread
+ executor = Runnable::run;
+ } else if (cleanupThread) {
executor = ForkJoinPool.commonPool();
Review comment:
You kind of narrowly dodge this being part of this issue, so more of an FYI, I'll second Ben's comments on this common thread pool usage.
It's a trappy and confusing use of common pool indicated to only impact a "cleanup" thread.
This is in fact a general executor for Caffeine, not simply the cleanup thread pool. In most cases, that is not a big deal, but the common pool is extremely easy to put into thread starvation, especially the common pool, if you perform any blocking tasks on it. There is an ugly workaround where you can indicate to the pool that you are going to block so it can compensate while you do so, but in general this pool is entirely meant for non blocking async type tasks. Where it get's trappy is that it's also used to do the work of loading for the async cache, which will often be doing IO, which is blocking. In this issue, async will use the current thread, so the case is avoid, but the code remains unclear and trappy and Caffeine can shift on where that executor is used in future versions given it's not a "cleanup" executor.
##########
File path: solr/core/src/java/org/apache/solr/response/transform/ChildDocTransformerFactory.java
##########
@@ -179,7 +179,11 @@ public static BitSetProducer getCachedBitSetProducer(final SolrQueryRequest requ
SolrCache<Query, BitSetProducer> parentCache = request.getSearcher().getCache(CACHE_NAME);
// lazily retrieve from solr cache
if (parentCache != null) {
- return parentCache.computeIfAbsent(query, QueryBitSetProducer::new);
+ try {
+ return parentCache.computeIfAbsent(query, QueryBitSetProducer::new);
+ } catch (IOException e) {
+ throw new AssertionError("Can't happen");
Review comment:
Not a fan of this myself.ll Impls change, new impls are added, it's declared to throw an IOException. Personally, I'd just turn it into a runtime io exception.
##########
File path: solr/solr-ref-guide/src/caches-warming.adoc
##########
@@ -79,6 +79,9 @@ Reasonable values, depending on the query volume and patterns, may lie somewhere
The `maxRamMB` attribute limits the maximum amount of memory a cache may consume.
When both `size` and `maxRamMB` limits are specified the `maxRamMB` limit will take precedence and the `size` limit will be ignored.
+The `async` attribute determines whether the cache stores direct results (`async=false`, the default) or whether it will store indirect references to the computation (`async=true`). Enabling `async` will use more slightly more memory per cache entry, but may result in reduced CPU cycles generating results or doing garbage collection. The most noticable improvements will be seen when there are many queries racing to compute the same results before they are inserted into the cache. In some use cases, increasing autowarming may be a better alternative to enabling an async cache. Additionally, an async cache will not prevent a data race for time-limited queries, as those may return differing sets of partial results.
Review comment:
Nothing urgent here, but personally I'd like it to also be a little more clear that this issue can help solve the problem in the description of the JIRA. It's somewhat referenced with the mention of autowarming, but with no history of that issue, I'd have no idea that using this would help prevent that problem or why autowarming was someone another approach to the async option.
##########
File path: solr/core/src/test/org/apache/solr/search/TestSolrCachePerf.java
##########
@@ -124,11 +129,10 @@ private void doTestGetPutCompute(Map<String, SummaryStatistics> ratioStats, Map<
}
}
Thread.yield();
- stopLatch.countDown();
+ stopLatch.countDown(); // Does this need to be in a finally block?
Review comment:
I would put it in one - there is no time out await below on that latch.
##########
File path: solr/benchmark/src/java/org/apache/solr/bench/search/FilterCache.java
##########
@@ -0,0 +1,148 @@
+/*
+ * 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.bench.search;
+
+import static org.apache.solr.bench.generators.SourceDSL.integers;
+
+import java.io.IOException;
+import java.net.HttpURLConnection;
+import java.net.URI;
+import java.nio.charset.StandardCharsets;
+import org.apache.solr.bench.BaseBenchState;
+import org.apache.solr.bench.Docs;
+import org.apache.solr.bench.MiniClusterState;
+import org.apache.solr.bench.generators.SolrGen;
+import org.apache.solr.client.solrj.SolrQuery;
+import org.apache.solr.client.solrj.SolrServerException;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.QueryRequest;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Param;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.TearDown;
+import org.openjdk.jmh.annotations.Threads;
+import org.openjdk.jmh.annotations.Warmup;
+import org.quicktheories.core.RandomnessSource;
+import org.quicktheories.impl.Constraint;
+
+@Fork(value = 1)
Review comment:
Overall, great benchmark. I do have some lessor issues with it, but I don't want to bog down this issue on it, I would not deal with anything in the benchmark here as it would hold up getting the issue into the release.
- On my desktop, the default run is fairly long. I have not finished the module doc, but I'd like to push default runs to be 10 minutes or less at most as you can easily crank things up or down as desired, but someone should be able to kick off a benchmark and get some kind of relatively reasonable feedback within a reasonable time and if they need to really engage with a benchmark then you alter settings on the command line. With a lot of benchmarks, if we don't manage this, running them all by default will take a very, very long time. This benchmark takes a bit over 20 min on my desktop.
- I don't think Threads.MAX is a great default. For one, it can often be simpler and less messy to start looking at multi threaded benchmarks with a handful of threads vs a ton. But more importantly, that takes into account hyper threaded cores - so on my desktop that uses 32 threads and at parts of the benchmark run, that can really clobber my machine for random small stretches. Of course that's not a bad idea, but you can configure a run like that easily via command line - it would be nice if the default run did not clobber my machine and just used a reasonably low number of threads.
- On my initial inspection, the results are not very consistent. I have some ideas on why that may be, but we want to have consistent results across runs.
--
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
[GitHub] [solr] madrob commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r676963537
##########
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:
ConcurrentHashMap does locking on the Node entry, which is either the collision list (up to 8 keys) or a Fibonacci tree bin (when larger than 8 keys), so I think this risk of locking other consumers out is small, but not negligible. I've started moving toward a reservation node approach, which is similar in spirit, but hopefully much less complex in practice!
--
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
[GitHub] [solr] ben-manes commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
ben-manes commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r683821705
##########
File path: solr/core/src/java/org/apache/solr/search/CaffeineCache.java
##########
@@ -190,73 +191,72 @@ public V get(K key) {
return cache.getIfPresent(key);
}
- @Override
- public V computeIfAbsent(K key, IOFunction<? super K, ? extends V> mappingFunction) throws IOException {
- /*
- This block will get deleted before commit, but I'm leaving it in for now in case I have to go back to that apporach
+ private V computeAsync(K key, IOFunction<? super K, ? extends V> mappingFunction) throws IOException {
+ // First do a lookup first: optimistic non-locking path.
+ // The cache does this for us on get(K, Function), but not putIfAbsent
+ V value = get(key);
+ if (value != null) {
+ return value;
+ }
Review comment:
Stats gets very funky with a Map, e.g. what stats are recorded for `V replace(k, v)` and `boolean replace(k, old, new)`? In Guava we decided to not record stats for the `asMap()` view and not cherry-pick. That made sense since you could use `Cache.getIfPresent` and prefer a loading `get(k, loader)` over a `putIfAbsent`.
In Caffeine, I decided that `compute` was valuable enough to break that practice and record stats. I'd probably lean towards `asMap().get(key)` also recording because it's been a surprise that `getIfPresent` should be used instead, but we have Guava's history to justify keeping as is. Similarly putIfAbsent sounds good, but it gets too confusing about the "correct" stats. Thus the fallback is to tell users to not rely on ours and create their own to get what they deem to be the right metrics. Ours are therefore best effort, but fallible.
--
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
[GitHub] [solr] madrob merged pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob merged pull request #230:
URL: https://github.com/apache/solr/pull/230
--
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
[GitHub] [solr] ben-manes commented on pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
ben-manes commented on pull request #230:
URL: https://github.com/apache/solr/pull/230#issuecomment-888786654
The common pool is a nice default, but due to blocking I/O it is easy for the workers to be saturated and the system idle. You might want a dedicated pool to avoid surprising degradations due to foreign code's usage of this shared resource. Otherwise, if you wanted to avoid running the future on a thread pool and use a caller thread, you could complete the future manually using a similar pattern to your original code.
```java
var future = new CompletableFuture<V>();
var result = asyncCache.asMap().putIfAbsent(key, future);
if (result != null) {
return result.join();
}
try {
V value = // do work
future.complete(value);
return value;
} catch (Throwable t) {
future.completeExceptionally(t);
throw t;
}
```
--
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
[GitHub] [solr] madrob commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r683771913
##########
File path: solr/core/src/java/org/apache/solr/search/CaffeineCache.java
##########
@@ -176,33 +193,92 @@ public V get(K key) {
return cache.getIfPresent(key);
}
- @Override
- public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) {
- return cache.get(key, k -> {
- inserts.increment();
- V value = mappingFunction.apply(k);
- if (value == null) {
- return null;
+ private V computeAsync(K key, IOFunction<? super K, ? extends V> mappingFunction) throws IOException {
+ CompletableFuture<V> future = new CompletableFuture<>();
+ CompletableFuture<V> result = asyncCache.asMap().putIfAbsent(key, future);
+ if (result != null) {
+ try {
+ // Another thread is already working on this computation, wait for them to finish
+ return result.join();
Review comment:
@ben-manes this code path doesn't seem to record a hit in the cache stats for the "happy" case where the result is already completed normally. should I be using a different API?
--
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
[GitHub] [solr] ben-manes commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
ben-manes commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r683832835
##########
File path: solr/core/src/java/org/apache/solr/search/CaffeineCache.java
##########
@@ -176,33 +193,92 @@ public V get(K key) {
return cache.getIfPresent(key);
}
- @Override
- public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) {
- return cache.get(key, k -> {
- inserts.increment();
- V value = mappingFunction.apply(k);
- if (value == null) {
- return null;
+ private V computeAsync(K key, IOFunction<? super K, ? extends V> mappingFunction) throws IOException {
+ CompletableFuture<V> future = new CompletableFuture<>();
+ CompletableFuture<V> result = asyncCache.asMap().putIfAbsent(key, future);
+ if (result != null) {
+ try {
+ // Another thread is already working on this computation, wait for them to finish
+ return result.join();
Review comment:
I think stats on an async cache, especially with the `asMap()` view, is confusing. I don't know if I got it instrumented right. It tries to be good enough but either requires a deeper review to fix holes, or custom stats to match your desired reporting.
--
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
[GitHub] [solr] markrmiller commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
markrmiller commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r706257640
##########
File path: solr/core/src/java/org/apache/solr/response/transform/ChildDocTransformerFactory.java
##########
@@ -179,7 +179,11 @@ public static BitSetProducer getCachedBitSetProducer(final SolrQueryRequest requ
SolrCache<Query, BitSetProducer> parentCache = request.getSearcher().getCache(CACHE_NAME);
// lazily retrieve from solr cache
if (parentCache != null) {
- return parentCache.computeIfAbsent(query, QueryBitSetProducer::new);
+ try {
+ return parentCache.computeIfAbsent(query, QueryBitSetProducer::new);
+ } catch (IOException e) {
+ throw new AssertionError("Can't happen");
Review comment:
Not a fan of this myself. Impls change, new impls are added, it's declared to throw an IOException. Personally, I'd just turn it into a runtime io exception.
--
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
[GitHub] [solr] ben-manes commented on pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
ben-manes commented on pull request #230:
URL: https://github.com/apache/solr/pull/230#issuecomment-918712124
nice! I think you said earlier you would upgrade to 3.x after this was merged. Please do that if possible.
--
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
[GitHub] [solr] madrob commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r675200608
##########
File path: solr/core/src/java/org/apache/solr/search/facet/UnInvertedField.java
##########
@@ -605,80 +605,14 @@ 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());
+ try {
+ return cache.computeIfAbsent(field,
+ (IOFunction<? super String, ? extends UnInvertedField>) f -> new UnInvertedField(field, searcher)
+ );
+ } catch (IOException e) {
+ // Do we still need this, computeIfAbsent can throw IOException for us directly
+ throw new SolrException(ErrorCode.SERVER_ERROR, "Exception occurred during uninverting " + field, e);
Review comment:
This is inconsistent with the non-cache case.
--
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
[GitHub] [solr] madrob commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r683729013
##########
File path: solr/core/src/java/org/apache/solr/search/CaffeineCache.java
##########
@@ -190,73 +191,72 @@ public V get(K key) {
return cache.getIfPresent(key);
}
- @Override
- public V computeIfAbsent(K key, IOFunction<? super K, ? extends V> mappingFunction) throws IOException {
- /*
- This block will get deleted before commit, but I'm leaving it in for now in case I have to go back to that apporach
+ private V computeAsync(K key, IOFunction<? super K, ? extends V> mappingFunction) throws IOException {
+ // First do a lookup first: optimistic non-locking path.
+ // The cache does this for us on get(K, Function), but not putIfAbsent
+ V value = get(key);
+ if (value != null) {
+ return value;
+ }
Review comment:
It looks like there's an interesting discrepancy here still, where the get from compute will increment the cache lookups stat, while the get in put will not increment the stat. I'm reading the source for 2.9.4 - I don't think we're ready to go to 3.x yet, although I'll probably do that as soon as I'm done with this issue.
--
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
[GitHub] [solr] markrmiller commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by 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
[GitHub] [solr] madrob commented on pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on pull request #230:
URL: https://github.com/apache/solr/pull/230#issuecomment-888736098
I was reading https://github.com/ben-manes/caffeine/blob/master/caffeine/src/main/java/com/github/benmanes/caffeine/cache/LocalAsyncCache.java#L88-L96 to check this, but somehow convinced myself that because we return the future then we're not blocking on the CHM compute call. But with your comment I realize that in the same thread case we would always be getting a _completed_ future. Need to think some more about whether the common fork-join pool is appropriate here or not.
--
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
[GitHub] [solr] madrob commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r676963773
##########
File path: solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
##########
@@ -804,15 +807,29 @@ public BitDocSet getDocSetBits(Query q) throws IOException {
}
// only handle positive (non negative) queries
- DocSet getPositiveDocSet(Query q) throws IOException {
- DocSet answer;
- if (filterCache != null) {
- answer = filterCache.get(q);
- if (answer != null) return answer;
+ DocSet getPositiveDocSet(Query query) throws IOException {
+ // TODO duplicated code with getDocSet, can maybe also use FutureDocSet here
+ boolean doCache = filterCache != null;
Review comment:
Very possible that I had other changes here before and then I had to back some of them out due to failing tests
--
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
[GitHub] [solr] madrob commented on a change in pull request #230: SOLR-15555 Improved caching on FilterQuery
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #230:
URL: https://github.com/apache/solr/pull/230#discussion_r706347715
##########
File path: solr/core/src/java/org/apache/solr/search/CaffeineCache.java
##########
@@ -114,9 +126,12 @@ public Object init(Map<String, String> args, Object persistence, CacheRegenerato
str = args.get(MAX_RAM_MB_PARAM);
int maxRamMB = str == null ? -1 : Double.valueOf(str).intValue();
maxRamBytes = maxRamMB < 0 ? Long.MAX_VALUE : maxRamMB * 1024L * 1024L;
- str = args.get(CLEANUP_THREAD_PARAM);
- cleanupThread = str != null && Boolean.parseBoolean(str);
- if (cleanupThread) {
+ cleanupThread = Boolean.parseBoolean(args.get(CLEANUP_THREAD_PARAM));
+ async = Boolean.parseBoolean(args.get(ASYNC_PARAM));
+ if (async) {
+ // We record futures in the map to decrease bucket-lock contention, but need computation handled in same thread
+ executor = Runnable::run;
+ } else if (cleanupThread) {
executor = ForkJoinPool.commonPool();
Review comment:
Yea, I detour around it, and this issue existed previously. We might want to investigate it in a follow up issue, but I'd rather not get into it now.
--
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