You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "sgup432 (via GitHub)" <gi...@apache.org> on 2026/01/09 06:40:37 UTC
[PR] Refactor QueryCache to improve performance [lucene]
sgup432 opened a new pull request, #15558:
URL: https://github.com/apache/lucene/pull/15558
### Description
Related issue - https://github.com/apache/lucene/issues/14222
Major changes:
- Divided the cache into partitions(16 by default and configurable). With each partition having its own read/write lock to improve concurrency.
- Change the cache <key, value> structure from `Map<Segment, Map<Query, DocIdSet>>` to `Map<<Query, Segment>, DocIdSet>`. This was done so that a cache key is efficiently mapped to a specific partition.
- Cache clean up of stale keys(segment, queries) now happen in asynchronously in the background. Users have a way to pass a scheduledExceutor which can be used to run the background maintenance work.
- Made putIfAbsent(), and get() public so that they can be overridden if necessary. Also to help benchmark these in isolation for performance
- Added JMH benchmark. With zipf/uniform distribution and put/get/invalidation tested.
<!--
If this is your first contribution to Lucene, please make sure you have reviewed the contribution guide.
https://github.com/apache/lucene/blob/main/CONTRIBUTING.md
-->
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "sgup432 (via GitHub)" <gi...@apache.org>.
sgup432 commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-4287816426
@msokolov
Few things to note about this change is that methods like onQueryCache/eviction in this cache have been deprecated. The methods are around listening to unique queries eviction or caching activity for cache stats. But with this change we don't have a way to know that as we key structure has changed. Though we still support it for backward compatibility but users won't get accurate stats. We have introduced new methods for stats now.
In addition, the cleanup has moved to a background thread, for that users need to pass a executor service or they can do manual cleanup. Otherwise by default, cleanups are disabled.
IMO So taking it in 11.0 might make sense as we are disabling cache by default and users can integrate when they want it and take new changes.
If you think the above changes are fine to be taken in 10.5, we can do that as well.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "vigyasharma (via GitHub)" <gi...@apache.org>.
vigyasharma commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-5025927364
@sgup432 Thanks for addressing this issue. It has been a major bottleneck with using the query cache. Were we able to back port it to 10.x? I don't think I see these changes in 10.5?
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "BaurzhanSakhariev (via GitHub)" <gi...@apache.org>.
BaurzhanSakhariev commented on code in PR #15558:
URL: https://github.com/apache/lucene/pull/15558#discussion_r2693966867
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -253,182 +304,63 @@ protected void onQueryEviction(Query query, long ramBytesUsed) {
* @see #onDocIdSetEviction
* @lucene.experimental
*/
- protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {
- assert writeLock.isHeldByCurrentThread();
- cacheSize += 1;
- cacheCount += 1;
- this.ramBytesUsed += ramBytesUsed;
- }
+ protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {}
/**
* Expert: callback when one or more {@link DocIdSet}s are removed from this cache.
*
* @see #onDocIdSetCache
* @lucene.experimental
*/
- protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sumRamBytesUsed) {
- assert writeLock.isHeldByCurrentThread();
- this.ramBytesUsed -= sumRamBytesUsed;
- cacheSize -= numEntries;
- }
+ protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sumRamBytesUsed) {}
/**
* Expert: callback when the cache is completely cleared.
*
* @lucene.experimental
*/
- protected void onClear() {
- assert writeLock.isHeldByCurrentThread();
- ramBytesUsed = 0;
- cacheSize = 0;
- }
-
- /** Whether evictions are required. */
- boolean requiresEviction() {
- assert writeLock.isHeldByCurrentThread();
- final int size = mostRecentlyUsedQueries.size();
- if (size == 0) {
- return false;
- } else {
- return size > maxSize || ramBytesUsed() > maxRamBytesUsed;
- }
- }
+ protected void onClear() {}
- CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
+ public CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
- final LeafCache leafCache = cache.get(readerKey);
- if (leafCache == null) {
- onMiss(readerKey, key);
- return null;
- }
- // this get call moves the query to the most-recently-used position
- final QueryMetadata record = uniqueQueries.get(key);
- if (record == null || record.query == null) {
- onMiss(readerKey, key);
- return null;
- }
- final CacheAndCount cached = leafCache.get(record.query);
- if (cached == null) {
- onMiss(readerKey, record.query);
- } else {
- onHit(readerKey, record.query);
- }
- return cached;
+ QueryCacheKey queryCacheKey = new QueryCacheKey(readerKey, key);
+ int partitionNumber = getPartitionNumber(queryCacheKey);
+ return this.lruQueryCachePartition[partitionNumber].get(queryCacheKey, key, cacheHelper);
}
public void putIfAbsent(Query query, CacheAndCount cached, IndexReader.CacheHelper cacheHelper) {
assert query instanceof BoostQuery == false;
assert query instanceof ConstantScoreQuery == false;
- // under a lock to make sure that mostRecentlyUsedQueries and cache remain sync'ed
- writeLock.lock();
- try {
- QueryMetadata record =
- uniqueQueries.computeIfAbsent(
- query,
- q -> {
- long queryRamBytesUsed = getRamBytesUsed(q);
- onQueryCache(q, queryRamBytesUsed);
- return new QueryMetadata(q, queryRamBytesUsed);
- });
- query = record.query;
- final IndexReader.CacheKey key = cacheHelper.getKey();
- LeafCache leafCache = cache.get(key);
- if (leafCache == null) {
- leafCache = new LeafCache(key);
- final LeafCache previous = cache.put(key, leafCache);
- ramBytesUsed += HASHTABLE_RAM_BYTES_PER_ENTRY;
- assert previous == null;
- // we just created a new leaf cache, need to register a close listener
- cacheHelper.addClosedListener(this::clearCoreCacheKey);
- }
- leafCache.putIfAbsent(query, cached);
- evictIfNecessary();
- } finally {
- writeLock.unlock();
- }
- }
-
- private void evictIfNecessary() {
- assert writeLock.isHeldByCurrentThread();
- // under a lock to make sure that mostRecentlyUsedQueries and cache keep sync'ed
- if (requiresEviction()) {
- Iterator<Map.Entry<Query, QueryMetadata>> iterator = uniqueQueries.entrySet().iterator();
- do {
- final Map.Entry<Query, QueryMetadata> entry = iterator.next();
- final int size = uniqueQueries.size();
- iterator.remove();
- if (size == uniqueQueries.size()) {
- // size did not decrease, because the hash of the query changed since it has been
- // put into the cache
- throw new ConcurrentModificationException(
- "Removal from the cache failed! This "
- + "is probably due to a query which has been modified after having been put into "
- + " the cache or a badly implemented clone(). Query class: ["
- + entry.getKey().getClass()
- + "], query: ["
- + entry.getKey()
- + "]");
- }
- onEviction(entry.getKey(), entry.getValue().queryRamBytesUsed);
- } while (iterator.hasNext() && requiresEviction());
- }
+ final IndexReader.CacheKey key = cacheHelper.getKey();
+ QueryCacheKey queryCacheKey = new QueryCacheKey(key, query);
+ int partitionNumber = getPartitionNumber(queryCacheKey);
+ this.lruQueryCachePartition[partitionNumber].putIfAbsent(
+ queryCacheKey, query, cacheHelper, cached);
}
/** Remove all cache entries for the given core cache key. */
- public void clearCoreCacheKey(Object coreKey) {
- writeLock.lock();
- try {
- final LeafCache leafCache = cache.remove(coreKey);
- if (leafCache != null) {
- ramBytesUsed -= HASHTABLE_RAM_BYTES_PER_ENTRY;
- final int numEntries = leafCache.cache.size();
- if (numEntries > 0) {
- onDocIdSetEviction(coreKey, numEntries, leafCache.ramBytesUsed);
- } else {
- assert numEntries == 0;
- assert leafCache.ramBytesUsed == 0;
- }
- }
- } finally {
- writeLock.unlock();
+ public void clearCoreCacheKey(IndexReader.CacheKey coreKey) {
+ this.registeredClosedListeners.remove(coreKey);
+ synchronized (keysToClean) {
+ keysToClean.add(coreKey);
Review Comment:
Sorry for chiming in here, just wanted to ask as I have been investigating OOM due to cache (https://github.com/crate/crate/pull/18913):
Do I get it right, that in the revision before this PR (`uniqueQueries`)
and with this PR (multiple `LRUQueryCachePartition`-s),
`clearCoreCacheKey` doesn't remove unique queries, ie doesn't remove `uniqueQueries` or `LRUQueryCachePartition.uniqueCacheKeys`?
I know that we can't remove `uniqueQuery` on `coreCacheKey` clearance because it can be used in other `LeafCache`(main)/other partitions(this PR).
However, it should be possible to remove if it's the last one across all leafCaches/all partitions? Probably "last one" can be checked by adding query occurrences to the `QueryMetadata` and using `clearQuery` when `clearCoreCacheKey` decrements occurrences to 0.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "sgup432 (via GitHub)" <gi...@apache.org>.
sgup432 commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-5027277004
>Were we able to back port it to 10.x? I don't think I see these changes in 10.5?
No, we didn't backport it to 10.x. I think it was a conscious choice as the query cache re-implementation introduced some breaking changes. For example onQueryCache/eviction methods in this cache have been deprecated and new methods have been introduced. They still work but won't report accurate data. This was done as query cache <key, value> structure was changed, and we don't track unique queries anymore.
Along with it, we also changed the cleanup logic of the cache from sync to async flow using a background thread. Note that this cleanup logic is around removing stale segment entries from the cache. Earlier it was done as soon as the segment got stale, now we collect those stale segment entries and clean up them later with a background thread. With this, user can either do this manually by calling cleanup() method whenever they want or pass a executor service to configure this cleanup at a specified interval.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "BaurzhanSakhariev (via GitHub)" <gi...@apache.org>.
BaurzhanSakhariev commented on code in PR #15558:
URL: https://github.com/apache/lucene/pull/15558#discussion_r2700886956
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -253,182 +304,63 @@ protected void onQueryEviction(Query query, long ramBytesUsed) {
* @see #onDocIdSetEviction
* @lucene.experimental
*/
- protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {
- assert writeLock.isHeldByCurrentThread();
- cacheSize += 1;
- cacheCount += 1;
- this.ramBytesUsed += ramBytesUsed;
- }
+ protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {}
/**
* Expert: callback when one or more {@link DocIdSet}s are removed from this cache.
*
* @see #onDocIdSetCache
* @lucene.experimental
*/
- protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sumRamBytesUsed) {
- assert writeLock.isHeldByCurrentThread();
- this.ramBytesUsed -= sumRamBytesUsed;
- cacheSize -= numEntries;
- }
+ protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sumRamBytesUsed) {}
/**
* Expert: callback when the cache is completely cleared.
*
* @lucene.experimental
*/
- protected void onClear() {
- assert writeLock.isHeldByCurrentThread();
- ramBytesUsed = 0;
- cacheSize = 0;
- }
-
- /** Whether evictions are required. */
- boolean requiresEviction() {
- assert writeLock.isHeldByCurrentThread();
- final int size = mostRecentlyUsedQueries.size();
- if (size == 0) {
- return false;
- } else {
- return size > maxSize || ramBytesUsed() > maxRamBytesUsed;
- }
- }
+ protected void onClear() {}
- CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
+ public CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
- final LeafCache leafCache = cache.get(readerKey);
- if (leafCache == null) {
- onMiss(readerKey, key);
- return null;
- }
- // this get call moves the query to the most-recently-used position
- final QueryMetadata record = uniqueQueries.get(key);
- if (record == null || record.query == null) {
- onMiss(readerKey, key);
- return null;
- }
- final CacheAndCount cached = leafCache.get(record.query);
- if (cached == null) {
- onMiss(readerKey, record.query);
- } else {
- onHit(readerKey, record.query);
- }
- return cached;
+ QueryCacheKey queryCacheKey = new QueryCacheKey(readerKey, key);
+ int partitionNumber = getPartitionNumber(queryCacheKey);
+ return this.lruQueryCachePartition[partitionNumber].get(queryCacheKey, key, cacheHelper);
}
public void putIfAbsent(Query query, CacheAndCount cached, IndexReader.CacheHelper cacheHelper) {
assert query instanceof BoostQuery == false;
assert query instanceof ConstantScoreQuery == false;
- // under a lock to make sure that mostRecentlyUsedQueries and cache remain sync'ed
- writeLock.lock();
- try {
- QueryMetadata record =
- uniqueQueries.computeIfAbsent(
- query,
- q -> {
- long queryRamBytesUsed = getRamBytesUsed(q);
- onQueryCache(q, queryRamBytesUsed);
- return new QueryMetadata(q, queryRamBytesUsed);
- });
- query = record.query;
- final IndexReader.CacheKey key = cacheHelper.getKey();
- LeafCache leafCache = cache.get(key);
- if (leafCache == null) {
- leafCache = new LeafCache(key);
- final LeafCache previous = cache.put(key, leafCache);
- ramBytesUsed += HASHTABLE_RAM_BYTES_PER_ENTRY;
- assert previous == null;
- // we just created a new leaf cache, need to register a close listener
- cacheHelper.addClosedListener(this::clearCoreCacheKey);
- }
- leafCache.putIfAbsent(query, cached);
- evictIfNecessary();
- } finally {
- writeLock.unlock();
- }
- }
-
- private void evictIfNecessary() {
- assert writeLock.isHeldByCurrentThread();
- // under a lock to make sure that mostRecentlyUsedQueries and cache keep sync'ed
- if (requiresEviction()) {
- Iterator<Map.Entry<Query, QueryMetadata>> iterator = uniqueQueries.entrySet().iterator();
- do {
- final Map.Entry<Query, QueryMetadata> entry = iterator.next();
- final int size = uniqueQueries.size();
- iterator.remove();
- if (size == uniqueQueries.size()) {
- // size did not decrease, because the hash of the query changed since it has been
- // put into the cache
- throw new ConcurrentModificationException(
- "Removal from the cache failed! This "
- + "is probably due to a query which has been modified after having been put into "
- + " the cache or a badly implemented clone(). Query class: ["
- + entry.getKey().getClass()
- + "], query: ["
- + entry.getKey()
- + "]");
- }
- onEviction(entry.getKey(), entry.getValue().queryRamBytesUsed);
- } while (iterator.hasNext() && requiresEviction());
- }
+ final IndexReader.CacheKey key = cacheHelper.getKey();
+ QueryCacheKey queryCacheKey = new QueryCacheKey(key, query);
+ int partitionNumber = getPartitionNumber(queryCacheKey);
+ this.lruQueryCachePartition[partitionNumber].putIfAbsent(
+ queryCacheKey, query, cacheHelper, cached);
}
/** Remove all cache entries for the given core cache key. */
- public void clearCoreCacheKey(Object coreKey) {
- writeLock.lock();
- try {
- final LeafCache leafCache = cache.remove(coreKey);
- if (leafCache != null) {
- ramBytesUsed -= HASHTABLE_RAM_BYTES_PER_ENTRY;
- final int numEntries = leafCache.cache.size();
- if (numEntries > 0) {
- onDocIdSetEviction(coreKey, numEntries, leafCache.ramBytesUsed);
- } else {
- assert numEntries == 0;
- assert leafCache.ramBytesUsed == 0;
- }
- }
- } finally {
- writeLock.unlock();
+ public void clearCoreCacheKey(IndexReader.CacheKey coreKey) {
+ this.registeredClosedListeners.remove(coreKey);
+ synchronized (keysToClean) {
+ keysToClean.add(coreKey);
Review Comment:
Thanks for clarifications!
If I understand it correctly, this PR also fixes a potential problem with custom `Query` implementations.
On the current `main` branch users have to ensure that `SomeCustomQuery.ramBytesUsed()` accounts for all its dependencies, which is BTW not always possible/feasible.
Otherwise, if `clearQuery` is not used, it's possible to have `maxSize-1` `SomeCustomQuery` instances sitting in `uniqueQueries` and retaining all their heavy dependencies, preventing them garbage collected and even causing OOM (if dependencies are heavy enough).
With this PR, if I understand correctly, it's guaranteed that `SomeCustomQuery` will be gone at some point (segment will be closed) regardless of ram computing.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "sgup432 (via GitHub)" <gi...@apache.org>.
sgup432 commented on code in PR #15558:
URL: https://github.com/apache/lucene/pull/15558#discussion_r2695478747
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -253,182 +304,63 @@ protected void onQueryEviction(Query query, long ramBytesUsed) {
* @see #onDocIdSetEviction
* @lucene.experimental
*/
- protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {
- assert writeLock.isHeldByCurrentThread();
- cacheSize += 1;
- cacheCount += 1;
- this.ramBytesUsed += ramBytesUsed;
- }
+ protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {}
/**
* Expert: callback when one or more {@link DocIdSet}s are removed from this cache.
*
* @see #onDocIdSetCache
* @lucene.experimental
*/
- protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sumRamBytesUsed) {
- assert writeLock.isHeldByCurrentThread();
- this.ramBytesUsed -= sumRamBytesUsed;
- cacheSize -= numEntries;
- }
+ protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sumRamBytesUsed) {}
/**
* Expert: callback when the cache is completely cleared.
*
* @lucene.experimental
*/
- protected void onClear() {
- assert writeLock.isHeldByCurrentThread();
- ramBytesUsed = 0;
- cacheSize = 0;
- }
-
- /** Whether evictions are required. */
- boolean requiresEviction() {
- assert writeLock.isHeldByCurrentThread();
- final int size = mostRecentlyUsedQueries.size();
- if (size == 0) {
- return false;
- } else {
- return size > maxSize || ramBytesUsed() > maxRamBytesUsed;
- }
- }
+ protected void onClear() {}
- CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
+ public CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
- final LeafCache leafCache = cache.get(readerKey);
- if (leafCache == null) {
- onMiss(readerKey, key);
- return null;
- }
- // this get call moves the query to the most-recently-used position
- final QueryMetadata record = uniqueQueries.get(key);
- if (record == null || record.query == null) {
- onMiss(readerKey, key);
- return null;
- }
- final CacheAndCount cached = leafCache.get(record.query);
- if (cached == null) {
- onMiss(readerKey, record.query);
- } else {
- onHit(readerKey, record.query);
- }
- return cached;
+ QueryCacheKey queryCacheKey = new QueryCacheKey(readerKey, key);
+ int partitionNumber = getPartitionNumber(queryCacheKey);
+ return this.lruQueryCachePartition[partitionNumber].get(queryCacheKey, key, cacheHelper);
}
public void putIfAbsent(Query query, CacheAndCount cached, IndexReader.CacheHelper cacheHelper) {
assert query instanceof BoostQuery == false;
assert query instanceof ConstantScoreQuery == false;
- // under a lock to make sure that mostRecentlyUsedQueries and cache remain sync'ed
- writeLock.lock();
- try {
- QueryMetadata record =
- uniqueQueries.computeIfAbsent(
- query,
- q -> {
- long queryRamBytesUsed = getRamBytesUsed(q);
- onQueryCache(q, queryRamBytesUsed);
- return new QueryMetadata(q, queryRamBytesUsed);
- });
- query = record.query;
- final IndexReader.CacheKey key = cacheHelper.getKey();
- LeafCache leafCache = cache.get(key);
- if (leafCache == null) {
- leafCache = new LeafCache(key);
- final LeafCache previous = cache.put(key, leafCache);
- ramBytesUsed += HASHTABLE_RAM_BYTES_PER_ENTRY;
- assert previous == null;
- // we just created a new leaf cache, need to register a close listener
- cacheHelper.addClosedListener(this::clearCoreCacheKey);
- }
- leafCache.putIfAbsent(query, cached);
- evictIfNecessary();
- } finally {
- writeLock.unlock();
- }
- }
-
- private void evictIfNecessary() {
- assert writeLock.isHeldByCurrentThread();
- // under a lock to make sure that mostRecentlyUsedQueries and cache keep sync'ed
- if (requiresEviction()) {
- Iterator<Map.Entry<Query, QueryMetadata>> iterator = uniqueQueries.entrySet().iterator();
- do {
- final Map.Entry<Query, QueryMetadata> entry = iterator.next();
- final int size = uniqueQueries.size();
- iterator.remove();
- if (size == uniqueQueries.size()) {
- // size did not decrease, because the hash of the query changed since it has been
- // put into the cache
- throw new ConcurrentModificationException(
- "Removal from the cache failed! This "
- + "is probably due to a query which has been modified after having been put into "
- + " the cache or a badly implemented clone(). Query class: ["
- + entry.getKey().getClass()
- + "], query: ["
- + entry.getKey()
- + "]");
- }
- onEviction(entry.getKey(), entry.getValue().queryRamBytesUsed);
- } while (iterator.hasNext() && requiresEviction());
- }
+ final IndexReader.CacheKey key = cacheHelper.getKey();
+ QueryCacheKey queryCacheKey = new QueryCacheKey(key, query);
+ int partitionNumber = getPartitionNumber(queryCacheKey);
+ this.lruQueryCachePartition[partitionNumber].putIfAbsent(
+ queryCacheKey, query, cacheHelper, cached);
}
/** Remove all cache entries for the given core cache key. */
- public void clearCoreCacheKey(Object coreKey) {
- writeLock.lock();
- try {
- final LeafCache leafCache = cache.remove(coreKey);
- if (leafCache != null) {
- ramBytesUsed -= HASHTABLE_RAM_BYTES_PER_ENTRY;
- final int numEntries = leafCache.cache.size();
- if (numEntries > 0) {
- onDocIdSetEviction(coreKey, numEntries, leafCache.ramBytesUsed);
- } else {
- assert numEntries == 0;
- assert leafCache.ramBytesUsed == 0;
- }
- }
- } finally {
- writeLock.unlock();
+ public void clearCoreCacheKey(IndexReader.CacheKey coreKey) {
+ this.registeredClosedListeners.remove(coreKey);
+ synchronized (keysToClean) {
+ keysToClean.add(coreKey);
Review Comment:
Before this PR, yes we don't remove unique queries from `uniqueQueries` because of the reason you mentioned. They can be either removed explicitly via `clearQuery` or automatically through LRU policy(inside `evictIfNecessary`).
With this PR, we are not tracking unique queries anymore. Instead this cache now a structure as `<<Query, Segment>, Value>` where key is a composite key and maxSize represents total keys a cache can hold. So if a segment is closed, then any keys containing such stale segments would be removed(along with associated query).
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "BaurzhanSakhariev (via GitHub)" <gi...@apache.org>.
BaurzhanSakhariev commented on code in PR #15558:
URL: https://github.com/apache/lucene/pull/15558#discussion_r2693966867
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -253,182 +304,63 @@ protected void onQueryEviction(Query query, long ramBytesUsed) {
* @see #onDocIdSetEviction
* @lucene.experimental
*/
- protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {
- assert writeLock.isHeldByCurrentThread();
- cacheSize += 1;
- cacheCount += 1;
- this.ramBytesUsed += ramBytesUsed;
- }
+ protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {}
/**
* Expert: callback when one or more {@link DocIdSet}s are removed from this cache.
*
* @see #onDocIdSetCache
* @lucene.experimental
*/
- protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sumRamBytesUsed) {
- assert writeLock.isHeldByCurrentThread();
- this.ramBytesUsed -= sumRamBytesUsed;
- cacheSize -= numEntries;
- }
+ protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sumRamBytesUsed) {}
/**
* Expert: callback when the cache is completely cleared.
*
* @lucene.experimental
*/
- protected void onClear() {
- assert writeLock.isHeldByCurrentThread();
- ramBytesUsed = 0;
- cacheSize = 0;
- }
-
- /** Whether evictions are required. */
- boolean requiresEviction() {
- assert writeLock.isHeldByCurrentThread();
- final int size = mostRecentlyUsedQueries.size();
- if (size == 0) {
- return false;
- } else {
- return size > maxSize || ramBytesUsed() > maxRamBytesUsed;
- }
- }
+ protected void onClear() {}
- CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
+ public CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
- final LeafCache leafCache = cache.get(readerKey);
- if (leafCache == null) {
- onMiss(readerKey, key);
- return null;
- }
- // this get call moves the query to the most-recently-used position
- final QueryMetadata record = uniqueQueries.get(key);
- if (record == null || record.query == null) {
- onMiss(readerKey, key);
- return null;
- }
- final CacheAndCount cached = leafCache.get(record.query);
- if (cached == null) {
- onMiss(readerKey, record.query);
- } else {
- onHit(readerKey, record.query);
- }
- return cached;
+ QueryCacheKey queryCacheKey = new QueryCacheKey(readerKey, key);
+ int partitionNumber = getPartitionNumber(queryCacheKey);
+ return this.lruQueryCachePartition[partitionNumber].get(queryCacheKey, key, cacheHelper);
}
public void putIfAbsent(Query query, CacheAndCount cached, IndexReader.CacheHelper cacheHelper) {
assert query instanceof BoostQuery == false;
assert query instanceof ConstantScoreQuery == false;
- // under a lock to make sure that mostRecentlyUsedQueries and cache remain sync'ed
- writeLock.lock();
- try {
- QueryMetadata record =
- uniqueQueries.computeIfAbsent(
- query,
- q -> {
- long queryRamBytesUsed = getRamBytesUsed(q);
- onQueryCache(q, queryRamBytesUsed);
- return new QueryMetadata(q, queryRamBytesUsed);
- });
- query = record.query;
- final IndexReader.CacheKey key = cacheHelper.getKey();
- LeafCache leafCache = cache.get(key);
- if (leafCache == null) {
- leafCache = new LeafCache(key);
- final LeafCache previous = cache.put(key, leafCache);
- ramBytesUsed += HASHTABLE_RAM_BYTES_PER_ENTRY;
- assert previous == null;
- // we just created a new leaf cache, need to register a close listener
- cacheHelper.addClosedListener(this::clearCoreCacheKey);
- }
- leafCache.putIfAbsent(query, cached);
- evictIfNecessary();
- } finally {
- writeLock.unlock();
- }
- }
-
- private void evictIfNecessary() {
- assert writeLock.isHeldByCurrentThread();
- // under a lock to make sure that mostRecentlyUsedQueries and cache keep sync'ed
- if (requiresEviction()) {
- Iterator<Map.Entry<Query, QueryMetadata>> iterator = uniqueQueries.entrySet().iterator();
- do {
- final Map.Entry<Query, QueryMetadata> entry = iterator.next();
- final int size = uniqueQueries.size();
- iterator.remove();
- if (size == uniqueQueries.size()) {
- // size did not decrease, because the hash of the query changed since it has been
- // put into the cache
- throw new ConcurrentModificationException(
- "Removal from the cache failed! This "
- + "is probably due to a query which has been modified after having been put into "
- + " the cache or a badly implemented clone(). Query class: ["
- + entry.getKey().getClass()
- + "], query: ["
- + entry.getKey()
- + "]");
- }
- onEviction(entry.getKey(), entry.getValue().queryRamBytesUsed);
- } while (iterator.hasNext() && requiresEviction());
- }
+ final IndexReader.CacheKey key = cacheHelper.getKey();
+ QueryCacheKey queryCacheKey = new QueryCacheKey(key, query);
+ int partitionNumber = getPartitionNumber(queryCacheKey);
+ this.lruQueryCachePartition[partitionNumber].putIfAbsent(
+ queryCacheKey, query, cacheHelper, cached);
}
/** Remove all cache entries for the given core cache key. */
- public void clearCoreCacheKey(Object coreKey) {
- writeLock.lock();
- try {
- final LeafCache leafCache = cache.remove(coreKey);
- if (leafCache != null) {
- ramBytesUsed -= HASHTABLE_RAM_BYTES_PER_ENTRY;
- final int numEntries = leafCache.cache.size();
- if (numEntries > 0) {
- onDocIdSetEviction(coreKey, numEntries, leafCache.ramBytesUsed);
- } else {
- assert numEntries == 0;
- assert leafCache.ramBytesUsed == 0;
- }
- }
- } finally {
- writeLock.unlock();
+ public void clearCoreCacheKey(IndexReader.CacheKey coreKey) {
+ this.registeredClosedListeners.remove(coreKey);
+ synchronized (keysToClean) {
+ keysToClean.add(coreKey);
Review Comment:
Sorry for chiming in here, just wanted to ask as I have been investigating OOM due to cache:
Do I get it right, that in the revision before this PR (`uniqueQueries`)
and with this PR (multiple `LRUQueryCachePartition`-s),
`clearCoreCacheKey` doesn't remove unique queries, ie doesn't remove `uniqueQueries` or `LRUQueryCachePartition.uniqueCacheKeys`?
I know that we can't remove `uniqueQuery` on `coreCacheKey` clearance because it can be used in other `LeafCache`(main)/other partitions(this PR).
However, it should be possible to remove if it's the last one across all leafCaches/all partitions? Probably "last one" can be checked by adding query occurrences to the `QueryMetadata` and using `clearQuery` when `clearCoreCacheKey` decrements occurrences to 0.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "msfroh (via GitHub)" <gi...@apache.org>.
msfroh commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-3747078175
Wow... @sgup432, those are some nice numbers. Any luck on reproducing and/or fixing the latest test failure?
```
ERROR: 1 test has failed:
- org.apache.lucene.search.TestLRUQueryCache.testDocValuesUpdatesDontBreakCache (:lucene:core)
Test output: /home/runner/work/lucene/lucene/lucene/core/build/test-results/test/outputs/OUTPUT-org.apache.lucene.search.TestLRUQueryCache.txt
Reproduce with: gradlew :lucene:core:test --tests "org.apache.lucene.search.TestLRUQueryCache.testDocValuesUpdatesDontBreakCache" -Ptests.asserts=true -Ptests.file.encoding=ISO-8859-1 -Ptests.gui=true -Ptests.jvmargs= -Ptests.jvms=4 -Ptests.seed=D906E38BEA18E902 -Ptests.vectorsize=256
```
I don't necessarily see how doc value updates would be affected by the structural part of your change.
Maybe it's the async cleanup? That test does seem to rely on items getting evicted from the cache because of the DV update.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "msokolov (via GitHub)" <gi...@apache.org>.
msokolov commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-4280244852
okay, I'll take a closer look, but if we think it's an outright win, we should probably backport it? Even if there are tradeoffs and we think it's better overall, we might want to do so. For example, this is what is happening with vector quantization -- the version in 10.4 is a complete overhaul of what we had in 10.3 and earlier that generally provides better recall but does do more indexing work and some users may see different results depending on their data.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "BaurzhanSakhariev (via GitHub)" <gi...@apache.org>.
BaurzhanSakhariev commented on code in PR #15558:
URL: https://github.com/apache/lucene/pull/15558#discussion_r2693966867
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -253,182 +304,63 @@ protected void onQueryEviction(Query query, long ramBytesUsed) {
* @see #onDocIdSetEviction
* @lucene.experimental
*/
- protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {
- assert writeLock.isHeldByCurrentThread();
- cacheSize += 1;
- cacheCount += 1;
- this.ramBytesUsed += ramBytesUsed;
- }
+ protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {}
/**
* Expert: callback when one or more {@link DocIdSet}s are removed from this cache.
*
* @see #onDocIdSetCache
* @lucene.experimental
*/
- protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sumRamBytesUsed) {
- assert writeLock.isHeldByCurrentThread();
- this.ramBytesUsed -= sumRamBytesUsed;
- cacheSize -= numEntries;
- }
+ protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sumRamBytesUsed) {}
/**
* Expert: callback when the cache is completely cleared.
*
* @lucene.experimental
*/
- protected void onClear() {
- assert writeLock.isHeldByCurrentThread();
- ramBytesUsed = 0;
- cacheSize = 0;
- }
-
- /** Whether evictions are required. */
- boolean requiresEviction() {
- assert writeLock.isHeldByCurrentThread();
- final int size = mostRecentlyUsedQueries.size();
- if (size == 0) {
- return false;
- } else {
- return size > maxSize || ramBytesUsed() > maxRamBytesUsed;
- }
- }
+ protected void onClear() {}
- CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
+ public CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
- final LeafCache leafCache = cache.get(readerKey);
- if (leafCache == null) {
- onMiss(readerKey, key);
- return null;
- }
- // this get call moves the query to the most-recently-used position
- final QueryMetadata record = uniqueQueries.get(key);
- if (record == null || record.query == null) {
- onMiss(readerKey, key);
- return null;
- }
- final CacheAndCount cached = leafCache.get(record.query);
- if (cached == null) {
- onMiss(readerKey, record.query);
- } else {
- onHit(readerKey, record.query);
- }
- return cached;
+ QueryCacheKey queryCacheKey = new QueryCacheKey(readerKey, key);
+ int partitionNumber = getPartitionNumber(queryCacheKey);
+ return this.lruQueryCachePartition[partitionNumber].get(queryCacheKey, key, cacheHelper);
}
public void putIfAbsent(Query query, CacheAndCount cached, IndexReader.CacheHelper cacheHelper) {
assert query instanceof BoostQuery == false;
assert query instanceof ConstantScoreQuery == false;
- // under a lock to make sure that mostRecentlyUsedQueries and cache remain sync'ed
- writeLock.lock();
- try {
- QueryMetadata record =
- uniqueQueries.computeIfAbsent(
- query,
- q -> {
- long queryRamBytesUsed = getRamBytesUsed(q);
- onQueryCache(q, queryRamBytesUsed);
- return new QueryMetadata(q, queryRamBytesUsed);
- });
- query = record.query;
- final IndexReader.CacheKey key = cacheHelper.getKey();
- LeafCache leafCache = cache.get(key);
- if (leafCache == null) {
- leafCache = new LeafCache(key);
- final LeafCache previous = cache.put(key, leafCache);
- ramBytesUsed += HASHTABLE_RAM_BYTES_PER_ENTRY;
- assert previous == null;
- // we just created a new leaf cache, need to register a close listener
- cacheHelper.addClosedListener(this::clearCoreCacheKey);
- }
- leafCache.putIfAbsent(query, cached);
- evictIfNecessary();
- } finally {
- writeLock.unlock();
- }
- }
-
- private void evictIfNecessary() {
- assert writeLock.isHeldByCurrentThread();
- // under a lock to make sure that mostRecentlyUsedQueries and cache keep sync'ed
- if (requiresEviction()) {
- Iterator<Map.Entry<Query, QueryMetadata>> iterator = uniqueQueries.entrySet().iterator();
- do {
- final Map.Entry<Query, QueryMetadata> entry = iterator.next();
- final int size = uniqueQueries.size();
- iterator.remove();
- if (size == uniqueQueries.size()) {
- // size did not decrease, because the hash of the query changed since it has been
- // put into the cache
- throw new ConcurrentModificationException(
- "Removal from the cache failed! This "
- + "is probably due to a query which has been modified after having been put into "
- + " the cache or a badly implemented clone(). Query class: ["
- + entry.getKey().getClass()
- + "], query: ["
- + entry.getKey()
- + "]");
- }
- onEviction(entry.getKey(), entry.getValue().queryRamBytesUsed);
- } while (iterator.hasNext() && requiresEviction());
- }
+ final IndexReader.CacheKey key = cacheHelper.getKey();
+ QueryCacheKey queryCacheKey = new QueryCacheKey(key, query);
+ int partitionNumber = getPartitionNumber(queryCacheKey);
+ this.lruQueryCachePartition[partitionNumber].putIfAbsent(
+ queryCacheKey, query, cacheHelper, cached);
}
/** Remove all cache entries for the given core cache key. */
- public void clearCoreCacheKey(Object coreKey) {
- writeLock.lock();
- try {
- final LeafCache leafCache = cache.remove(coreKey);
- if (leafCache != null) {
- ramBytesUsed -= HASHTABLE_RAM_BYTES_PER_ENTRY;
- final int numEntries = leafCache.cache.size();
- if (numEntries > 0) {
- onDocIdSetEviction(coreKey, numEntries, leafCache.ramBytesUsed);
- } else {
- assert numEntries == 0;
- assert leafCache.ramBytesUsed == 0;
- }
- }
- } finally {
- writeLock.unlock();
+ public void clearCoreCacheKey(IndexReader.CacheKey coreKey) {
+ this.registeredClosedListeners.remove(coreKey);
+ synchronized (keysToClean) {
+ keysToClean.add(coreKey);
Review Comment:
Sorry for chiming in here, just wanted to ask as I have been investigating OOM due to cache (https://github.com/crate/crate/pull/18913):
Do I get it right, that in the revision before this PR (`uniqueQueries`)
and with this PR (multiple `LRUQueryCachePartition`-s),
`clearCoreCacheKey` doesn't remove unique queries, ie doesn't remove `uniqueQueries` (main) or `LRUQueryCachePartition.uniqueCacheKeys` (this PR)?
I know that we can't remove `uniqueQuery` on `coreCacheKey` clearance because it can be used in other `LeafCache`(main)/other partitions(this PR).
However, it should be possible to remove if it's the last one across all leafCaches/all partitions? Probably "last one" can be checked by adding query occurrences to the `QueryMetadata` and using `clearQuery` when `clearCoreCacheKey` decrements occurrences to 0.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "msokolov (via GitHub)" <gi...@apache.org>.
msokolov commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-4273898684
Is there any reason this can't be backported to 10x branch?
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "sgup432 (via GitHub)" <gi...@apache.org>.
sgup432 commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-4294539898
Yeah without the cleanup, it will affect cache hit rates pretty badly.
This cleanup is around removing stale segment entries from the cache. So without this, you will have stale entries piling up until unless removed by the eviction policy(LRU).
Please note that without cleanup, cache still works functionally correct ie it never goes beyond > max_size or fetch stale entries.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "1ou (via GitHub)" <gi...@apache.org>.
1ou commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-4386270712
@sgup432 it would be great if you backport it 10.5
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "sgup432 (via GitHub)" <gi...@apache.org>.
sgup432 commented on code in PR #15558:
URL: https://github.com/apache/lucene/pull/15558#discussion_r2695478747
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -253,182 +304,63 @@ protected void onQueryEviction(Query query, long ramBytesUsed) {
* @see #onDocIdSetEviction
* @lucene.experimental
*/
- protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {
- assert writeLock.isHeldByCurrentThread();
- cacheSize += 1;
- cacheCount += 1;
- this.ramBytesUsed += ramBytesUsed;
- }
+ protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {}
/**
* Expert: callback when one or more {@link DocIdSet}s are removed from this cache.
*
* @see #onDocIdSetCache
* @lucene.experimental
*/
- protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sumRamBytesUsed) {
- assert writeLock.isHeldByCurrentThread();
- this.ramBytesUsed -= sumRamBytesUsed;
- cacheSize -= numEntries;
- }
+ protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sumRamBytesUsed) {}
/**
* Expert: callback when the cache is completely cleared.
*
* @lucene.experimental
*/
- protected void onClear() {
- assert writeLock.isHeldByCurrentThread();
- ramBytesUsed = 0;
- cacheSize = 0;
- }
-
- /** Whether evictions are required. */
- boolean requiresEviction() {
- assert writeLock.isHeldByCurrentThread();
- final int size = mostRecentlyUsedQueries.size();
- if (size == 0) {
- return false;
- } else {
- return size > maxSize || ramBytesUsed() > maxRamBytesUsed;
- }
- }
+ protected void onClear() {}
- CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
+ public CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
- final LeafCache leafCache = cache.get(readerKey);
- if (leafCache == null) {
- onMiss(readerKey, key);
- return null;
- }
- // this get call moves the query to the most-recently-used position
- final QueryMetadata record = uniqueQueries.get(key);
- if (record == null || record.query == null) {
- onMiss(readerKey, key);
- return null;
- }
- final CacheAndCount cached = leafCache.get(record.query);
- if (cached == null) {
- onMiss(readerKey, record.query);
- } else {
- onHit(readerKey, record.query);
- }
- return cached;
+ QueryCacheKey queryCacheKey = new QueryCacheKey(readerKey, key);
+ int partitionNumber = getPartitionNumber(queryCacheKey);
+ return this.lruQueryCachePartition[partitionNumber].get(queryCacheKey, key, cacheHelper);
}
public void putIfAbsent(Query query, CacheAndCount cached, IndexReader.CacheHelper cacheHelper) {
assert query instanceof BoostQuery == false;
assert query instanceof ConstantScoreQuery == false;
- // under a lock to make sure that mostRecentlyUsedQueries and cache remain sync'ed
- writeLock.lock();
- try {
- QueryMetadata record =
- uniqueQueries.computeIfAbsent(
- query,
- q -> {
- long queryRamBytesUsed = getRamBytesUsed(q);
- onQueryCache(q, queryRamBytesUsed);
- return new QueryMetadata(q, queryRamBytesUsed);
- });
- query = record.query;
- final IndexReader.CacheKey key = cacheHelper.getKey();
- LeafCache leafCache = cache.get(key);
- if (leafCache == null) {
- leafCache = new LeafCache(key);
- final LeafCache previous = cache.put(key, leafCache);
- ramBytesUsed += HASHTABLE_RAM_BYTES_PER_ENTRY;
- assert previous == null;
- // we just created a new leaf cache, need to register a close listener
- cacheHelper.addClosedListener(this::clearCoreCacheKey);
- }
- leafCache.putIfAbsent(query, cached);
- evictIfNecessary();
- } finally {
- writeLock.unlock();
- }
- }
-
- private void evictIfNecessary() {
- assert writeLock.isHeldByCurrentThread();
- // under a lock to make sure that mostRecentlyUsedQueries and cache keep sync'ed
- if (requiresEviction()) {
- Iterator<Map.Entry<Query, QueryMetadata>> iterator = uniqueQueries.entrySet().iterator();
- do {
- final Map.Entry<Query, QueryMetadata> entry = iterator.next();
- final int size = uniqueQueries.size();
- iterator.remove();
- if (size == uniqueQueries.size()) {
- // size did not decrease, because the hash of the query changed since it has been
- // put into the cache
- throw new ConcurrentModificationException(
- "Removal from the cache failed! This "
- + "is probably due to a query which has been modified after having been put into "
- + " the cache or a badly implemented clone(). Query class: ["
- + entry.getKey().getClass()
- + "], query: ["
- + entry.getKey()
- + "]");
- }
- onEviction(entry.getKey(), entry.getValue().queryRamBytesUsed);
- } while (iterator.hasNext() && requiresEviction());
- }
+ final IndexReader.CacheKey key = cacheHelper.getKey();
+ QueryCacheKey queryCacheKey = new QueryCacheKey(key, query);
+ int partitionNumber = getPartitionNumber(queryCacheKey);
+ this.lruQueryCachePartition[partitionNumber].putIfAbsent(
+ queryCacheKey, query, cacheHelper, cached);
}
/** Remove all cache entries for the given core cache key. */
- public void clearCoreCacheKey(Object coreKey) {
- writeLock.lock();
- try {
- final LeafCache leafCache = cache.remove(coreKey);
- if (leafCache != null) {
- ramBytesUsed -= HASHTABLE_RAM_BYTES_PER_ENTRY;
- final int numEntries = leafCache.cache.size();
- if (numEntries > 0) {
- onDocIdSetEviction(coreKey, numEntries, leafCache.ramBytesUsed);
- } else {
- assert numEntries == 0;
- assert leafCache.ramBytesUsed == 0;
- }
- }
- } finally {
- writeLock.unlock();
+ public void clearCoreCacheKey(IndexReader.CacheKey coreKey) {
+ this.registeredClosedListeners.remove(coreKey);
+ synchronized (keysToClean) {
+ keysToClean.add(coreKey);
Review Comment:
Before this PR, yes we don't remove unique queries from `uniqueQueries` because of the reason you mentioned. They can be either removed explicitly via `clearQuery` or automatically through LRU policy(inside `evictIfNecessary`).
With this PR, we are not tracking unique queries anymore. Instead cache now has a structure as `<<Query, Segment>, Value>` where key is a composite key and maxSize represents total keys a cache can hold. So if a segment is closed, then any keys containing such stale segments would be removed(along with associated query).
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-4128030707
`uniqueQueries` really does seem like the main synchronization bottle neck on read (if there aren't many cache updates occurring and thus causing all reads to wait).
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "msokolov (via GitHub)" <gi...@apache.org>.
msokolov commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-4287763261
okay, if there's no objection I'll try the backport and see how it goes
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #15558:
URL: https://github.com/apache/lucene/pull/15558#discussion_r3906003341
##########
lucene/core/src/java/org/apache/lucene/index/IndexReader.java:
##########
Review Comment:
Thank you for making this public. I have a need to create this. At present with Lucene 10, I have a hack in my code involving subclassing FilterDirectoryReader just to mint my own CacheKey instances.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-4210629591
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "sgup432 (via GitHub)" <gi...@apache.org>.
sgup432 commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-4121884743
I would like some help in reviewing this PR. We have seen multiple performance issues in the current implementation of query cache. And this PR would resolve all those issues plus make it much better.
One of the major issues we have in production is the high wait lock times:
<img width="813" height="53" alt="JFR_high_lock_1" src="https://github.com/user-attachments/assets/613ed452-bab3-4e2f-a921-adc4dda557b3" />
<img width="813" height="194" alt="JFR_high_lock" src="https://github.com/user-attachments/assets/f19726ea-ebf7-4b80-a46b-8ba1c562a1c6" />
This was one of the JFR dump taken for a particular OpenSearch customer who was facing tail latency spikes. We have seen multiple instances of the same issue.
The profile was taken for 5min duration and on one of their Opensearch data node. This shows the total lock wait time(>7mins across 97 distinct search threads) on this synchronized map get call which is coming from the original implementation of query cache.
Pinging folks to see if they can help here.
@msfroh @benwtrent
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "sgup432 (via GitHub)" <gi...@apache.org>.
sgup432 commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-4128193278
>is this trace/usage using the latest stuff?
Yeah this is using the latest stuff. As you mentioned, SynchronizedMap(ie uniqueQueries) independently maintans a global mutex lock for read/write, and we constantly keeping seeing this causing high lock wait times for high throughput workloads for a hot partition.
>I wonder if we can make that part better. The main reason its still a synchronized map is because LinkedHashMap isn't read threadsafe (e.g. even reads will update the inner structure).
I did try that ie having a custom LRU implementation but gains were not immense. I think the current approach this PR takes is to divide the cache into multiple partitions(similar to what guava cache does) and each partition has its own read/write lock. So overall for different queries, we go to a different partition(hashkey being "query + segment") which has improved the read/write throughput quite significantly(as seen in above benchmarks). And internally this has solved SynchronizedMap.get issue as well. Overall even without SynchronizedMap.get issues, I think this cache was not very performant in the first place IMO.
>uniqueQueries really does seem like the main synchronization bottle neck on read
Yes
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-3850435882
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "sgup432 (via GitHub)" <gi...@apache.org>.
sgup432 commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-3747262400
@msfroh Yeah I tried this test locally but it passed. It seems flaky.
You are right, it is not related to doc value updates as such, it might be very much due to async cleanup.
Let me try running this multiple times to reproduce and fix this.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-4128020754
@sgup432 is this trace/usage using the latest stuff? Cache was recently switched to a read/write locking structure that helped throughput IMMENSELY.
The only map that is synchronized in the structure is `uniqueQueries`.
I wonder if we can make that part better. The main reason its still a synchronized map is because LinkedHashMap isn't read threadsafe (e.g. even reads will update the inner structure).
I don't know of a better way to handle it though :/
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "msfroh (via GitHub)" <gi...@apache.org>.
msfroh merged PR #15558:
URL: https://github.com/apache/lucene/pull/15558
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "msfroh (via GitHub)" <gi...@apache.org>.
msfroh commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-4225322631
IMO, this change is pretty good as implemented. While I'm slightly nervous about the implications of modifying the query cache implementation, I think in conjunction with https://github.com/apache/lucene/pull/14187, the out-of-the-box experience will not be affected anyway. For those of us who do want a query cache, I think this one should perform much better in highly-concurrent environments.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "msokolov (via GitHub)" <gi...@apache.org>.
msokolov commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-4291699876
hmm that does give one pause -- the "cleanups are disabled" could bite someone if they are relying on it. I'm not sure I understand what cleanups are ... is there an idea that queries time out of the cache rather than being evicted by some other query?
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-4086637226
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "msfroh (via GitHub)" <gi...@apache.org>.
msfroh commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-4276457483
> Is there any reason this can't be backported to 10x branch?
As I mentioned in my last comment, "I think in conjunction with https://github.com/apache/lucene/pull/14187, the out-of-the-box experience will not be affected anyway." If we do backport it to 10x, it will affect the out-of-the-box experience, since the query cache is still on by default (and this change modifies that default implementation).
Functionally, it's probably fine, but I've been erring on the side of caution.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "sgup432 (via GitHub)" <gi...@apache.org>.
sgup432 commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-3727486033
Micro benchmarked putIfAbsent/get/invalidate
Benchmark | Baseline(V1) (ops/s) | New changes(V2) (ops/s) | Δ (V2 − V1) | % Change
-- | -- | -- | -- | --
concurrentGetAndPutsWithInvalidate_zipf (overall) | 95,641,994 | 122,292,131 | +26,650,137 | +27.9%
├─ get | 2,801,546 | 24,292,430 | +21,490,884 | +768% (8.7×)
├─ invalidate | 88,826,531 | 92,760,013 | +3,933,482 | +4.4%
└─ put | 4,013,916 | 5,239,688 | +1,225,772 | +30.5%
concurrentGetAndPuts_uniform (overall) | 4,124,777 | 14,596,955 | +10,472,178 | +254% (3.5×)
├─ get | 772,085 | 9,847,626 | +9,075,541 | +1,176% (12.8×)
└─ put | 3,352,692 | 4,749,328 | +1,396,636 | +41.7%
concurrentGetAndPuts_zipf (overall) | 5,551,209 | 15,477,692 | +9,926,483 | +179% (2.8×)
├─ get | 1,239,469 | 9,296,097 | +8,056,628 | +650% (7.5×)
└─ put | 4,311,740 | 6,181,595 | +1,869,855 | +43.4%
concurrent_puts | 4,829,739 | 21,427,524 | +16,597,785 | +344% (4.4×)
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "sgup432 (via GitHub)" <gi...@apache.org>.
sgup432 commented on PR #15558:
URL: https://github.com/apache/lucene/pull/15558#issuecomment-3752469966
Turns out that testDocValuesUpdatesDontBreakCache was failing due to cache keys getting evicted and causing cache misses.
Earlier, we initialized the cache with max size as 1. Not that earlier this denoted that the cache can hold at max 1 "query". Though overall cache can have multiple keys for the same query for different segments. Because of older structure <Segment, Map<Query, Value>>
Post this change, that contract is broken ie maxSize denotes the total keys a cache can hold. As we changed the structure to <<Query, Segment>, Value> for performance reasons.
The above test failed because by default we created 16 partitions, with each having 1 as maxSize. So at a certain point we expected that both segments value should be cached which it did not because a specific partition can hold 1 key at max, so there was a miss causing the test to fail.
Increased cache size to fix the failed test. Will also update documents to denote that maxSize number no-more represents the max query a cache can hold.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Refactor QueryCache to improve performance [lucene]
Posted by "sgup432 (via GitHub)" <gi...@apache.org>.
sgup432 commented on code in PR #15558:
URL: https://github.com/apache/lucene/pull/15558#discussion_r2706169037
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -253,182 +304,63 @@ protected void onQueryEviction(Query query, long ramBytesUsed) {
* @see #onDocIdSetEviction
* @lucene.experimental
*/
- protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {
- assert writeLock.isHeldByCurrentThread();
- cacheSize += 1;
- cacheCount += 1;
- this.ramBytesUsed += ramBytesUsed;
- }
+ protected void onDocIdSetCache(Object readerCoreKey, long ramBytesUsed) {}
/**
* Expert: callback when one or more {@link DocIdSet}s are removed from this cache.
*
* @see #onDocIdSetCache
* @lucene.experimental
*/
- protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sumRamBytesUsed) {
- assert writeLock.isHeldByCurrentThread();
- this.ramBytesUsed -= sumRamBytesUsed;
- cacheSize -= numEntries;
- }
+ protected void onDocIdSetEviction(Object readerCoreKey, int numEntries, long sumRamBytesUsed) {}
/**
* Expert: callback when the cache is completely cleared.
*
* @lucene.experimental
*/
- protected void onClear() {
- assert writeLock.isHeldByCurrentThread();
- ramBytesUsed = 0;
- cacheSize = 0;
- }
-
- /** Whether evictions are required. */
- boolean requiresEviction() {
- assert writeLock.isHeldByCurrentThread();
- final int size = mostRecentlyUsedQueries.size();
- if (size == 0) {
- return false;
- } else {
- return size > maxSize || ramBytesUsed() > maxRamBytesUsed;
- }
- }
+ protected void onClear() {}
- CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
+ public CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
- final LeafCache leafCache = cache.get(readerKey);
- if (leafCache == null) {
- onMiss(readerKey, key);
- return null;
- }
- // this get call moves the query to the most-recently-used position
- final QueryMetadata record = uniqueQueries.get(key);
- if (record == null || record.query == null) {
- onMiss(readerKey, key);
- return null;
- }
- final CacheAndCount cached = leafCache.get(record.query);
- if (cached == null) {
- onMiss(readerKey, record.query);
- } else {
- onHit(readerKey, record.query);
- }
- return cached;
+ QueryCacheKey queryCacheKey = new QueryCacheKey(readerKey, key);
+ int partitionNumber = getPartitionNumber(queryCacheKey);
+ return this.lruQueryCachePartition[partitionNumber].get(queryCacheKey, key, cacheHelper);
}
public void putIfAbsent(Query query, CacheAndCount cached, IndexReader.CacheHelper cacheHelper) {
assert query instanceof BoostQuery == false;
assert query instanceof ConstantScoreQuery == false;
- // under a lock to make sure that mostRecentlyUsedQueries and cache remain sync'ed
- writeLock.lock();
- try {
- QueryMetadata record =
- uniqueQueries.computeIfAbsent(
- query,
- q -> {
- long queryRamBytesUsed = getRamBytesUsed(q);
- onQueryCache(q, queryRamBytesUsed);
- return new QueryMetadata(q, queryRamBytesUsed);
- });
- query = record.query;
- final IndexReader.CacheKey key = cacheHelper.getKey();
- LeafCache leafCache = cache.get(key);
- if (leafCache == null) {
- leafCache = new LeafCache(key);
- final LeafCache previous = cache.put(key, leafCache);
- ramBytesUsed += HASHTABLE_RAM_BYTES_PER_ENTRY;
- assert previous == null;
- // we just created a new leaf cache, need to register a close listener
- cacheHelper.addClosedListener(this::clearCoreCacheKey);
- }
- leafCache.putIfAbsent(query, cached);
- evictIfNecessary();
- } finally {
- writeLock.unlock();
- }
- }
-
- private void evictIfNecessary() {
- assert writeLock.isHeldByCurrentThread();
- // under a lock to make sure that mostRecentlyUsedQueries and cache keep sync'ed
- if (requiresEviction()) {
- Iterator<Map.Entry<Query, QueryMetadata>> iterator = uniqueQueries.entrySet().iterator();
- do {
- final Map.Entry<Query, QueryMetadata> entry = iterator.next();
- final int size = uniqueQueries.size();
- iterator.remove();
- if (size == uniqueQueries.size()) {
- // size did not decrease, because the hash of the query changed since it has been
- // put into the cache
- throw new ConcurrentModificationException(
- "Removal from the cache failed! This "
- + "is probably due to a query which has been modified after having been put into "
- + " the cache or a badly implemented clone(). Query class: ["
- + entry.getKey().getClass()
- + "], query: ["
- + entry.getKey()
- + "]");
- }
- onEviction(entry.getKey(), entry.getValue().queryRamBytesUsed);
- } while (iterator.hasNext() && requiresEviction());
- }
+ final IndexReader.CacheKey key = cacheHelper.getKey();
+ QueryCacheKey queryCacheKey = new QueryCacheKey(key, query);
+ int partitionNumber = getPartitionNumber(queryCacheKey);
+ this.lruQueryCachePartition[partitionNumber].putIfAbsent(
+ queryCacheKey, query, cacheHelper, cached);
}
/** Remove all cache entries for the given core cache key. */
- public void clearCoreCacheKey(Object coreKey) {
- writeLock.lock();
- try {
- final LeafCache leafCache = cache.remove(coreKey);
- if (leafCache != null) {
- ramBytesUsed -= HASHTABLE_RAM_BYTES_PER_ENTRY;
- final int numEntries = leafCache.cache.size();
- if (numEntries > 0) {
- onDocIdSetEviction(coreKey, numEntries, leafCache.ramBytesUsed);
- } else {
- assert numEntries == 0;
- assert leafCache.ramBytesUsed == 0;
- }
- }
- } finally {
- writeLock.unlock();
+ public void clearCoreCacheKey(IndexReader.CacheKey coreKey) {
+ this.registeredClosedListeners.remove(coreKey);
+ synchronized (keysToClean) {
+ keysToClean.add(coreKey);
Review Comment:
>With this PR, if I understand correctly, it's guaranteed that SomeCustomQuery will be gone at some point (segment will be closed) regardless of ram computing.
Yes with this PR change, if a segment is closed, all of its associated queries will be deleted as well.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org