You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "boicehuang (via GitHub)" <gi...@apache.org> on 2024/04/15 11:57:51 UTC
[PR] Performance improvements to use read lock to access LRUQueryCache [lucene]
boicehuang opened a new pull request, #13306:
URL: https://github.com/apache/lucene/pull/13306
Elasticsearch (which based on lucene) can automatically infer types for users with its dynamic mapping feature. When users index some low cardinality fields, such as gender / age / status... they often use some numbers to represent the values, while ES will infer these fields as long, and ES uses BKD as the index of long fields.
Just as #541 said, when the data volume grows, building the result set of low-cardinality fields will make the CPU usage and load very high even if we use a boolean query with filter clauses for low-cardinality fields.
I found that one main reason is that it uses node-level exclusive ReentrantLock to limit accessing LRUQueryCache.
For low cardinality fields, QPS and costs of their queries are often very high, which often causes trying locking failures when obtaining cache, resulting in low concurrency in accessing the cache.
So I use a ReentrantReadWriteLock to replace ReentrantLock. When IndexSearcher needs to get the cache of an query, we only lock the read lock. In update or clear cache cases, the write lock is still used to lock to ensure exclusive use.
I mocked a field that has 10,000,000 docs per value and to ensure that most segments can be cached, I send forcemerge to merge small segments into several large segments before I search it with a 1 term PointInSetQuery, finally the request per second increased from 500 to 5000.
--
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] Performance improvements to use RWLock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on PR #13306:
URL: https://github.com/apache/lucene/pull/13306#issuecomment-2104395723
> I think this is ready for merging. I can do the merging, but won't back port to 9x until we see nightlies. They might catch something we missed.
>
> @boicehuang could you add a changes entry to 9.11 under optimizations?
Finished
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1581809438
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
In my optimization, the writing to `cache` object was still protected via the write lock, but reading `cache` object can be a concurrent action via making `cache` object to be `ConcurrentHashMap`. This is the key point of my optimization. I
am also trying to read through all the concurrency paths with a picture.
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
In my optimization, the writing to `cache` object was still protected via the write lock, but reading `cache` object can be a concurrent action via making `cache` object to be `ConcurrentHashMap`. This is the key point of my optimization. I am also trying to read through all the concurrency paths with a picture.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1582091772
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
I drew 2 flow charts and tried to describe all the concurrency paths.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1578336720
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
So, `Collections.synchronizedMap` should be used, or locking manually around this particular `get`.
And that means now we are locking on read again, which might mean all performance gains go away.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1584480939
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -628,7 +632,7 @@ private class LeafCache implements Accountable {
LeafCache(Object key) {
this.key = key;
- cache = new IdentityHashMap<>();
+ cache = Collections.synchronizedMap(new IdentityHashMap<>());
Review Comment:
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on PR #13306:
URL: https://github.com/apache/lucene/pull/13306#issuecomment-2072527542
This optimization also benefits high-cost querys such as terms query with 10000 terms, by reading cache more frequently instead of searching inverted index
doc count | field cardinality | query terms count | baseline QPS | candidate QPS | diff percentage
-- | -- | -- | -- | -- | --
30000000 | 1000000 | 10000 | 160 | 473 | 191.9%
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579458103
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
I understand your point. I try to use `Collections.synchronizedMap` and get the benchmark results as follows
doc count | field cardinality | query point | baseline QPS | candidate QPS | diff percentage
-- | -- | -- | -- | -- | --
30000000 | 10 | 1 | 2481 | 5102 | 105.6%
30000000 | 10 | 1 | 2481 | 4843 | 95.2% (using `LongAdder `)
30000000 | 10 | 1 | 2481 | 3851 | 55% (using `LongAdder ` And `ConcurrentHashMap`)
30000000 | 10 | 1 | 2481 | 3751 | 51% (using `LongAdder ` And `Collections.synchronizedMap`)
It seems that performance can be improved if the locking time is minimized.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579285322
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
In `ConcurrentHashMap`, although the order of insertion operations is uncertain, which leads to test failure, it ensures the thread safety of insertion operations and still has a 55% improvement.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1581809438
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
In my optimization, `uniqueQueries` is `Collections.synchronizedMap(LinkedHashMap)` to achieve LRU expiration, the writing to `cache` object was protected via the write lock, but reading `cache` object can be a concurrent action via making `cache` object to be `ConcurrentHashMap`. This is the key point of my optimization. I am also trying to read through all the concurrency paths.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on PR #13306:
URL: https://github.com/apache/lucene/pull/13306#issuecomment-2072528754
This optimization also benefits high-cost querys such as terms query with 10000 terms, by reading cache more frequently instead of searching inverted index
doc count | field cardinality | query terms count | baseline QPS | candidate QPS | diff percentage
-- | -- | -- | -- | -- | --
30000000 | 1000000 | 10000 | 160 | 473 | 191.9%
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579458103
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
I understand your point. I try to use `Collections.synchronizedMap` and get the benchmark results as follows
doc count | field cardinality | query point | baseline QPS | candidate QPS | diff percentage
-- | -- | -- | -- | -- | --
30000000 | 10 | 1 | 2481 | 5102 | 105.6%
30000000 | 10 | 1 | 2481 | 4843 | 95.2% (using `LongAdder `)
30000000 | 10 | 1 | 2481 | 3751 | 51% (using `LongAdder ` And `Collections.synchronizedMap`)
It seems that performance can be improved if the time spent occupying the lock is minimized.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1580286445
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
Yes, So I want to try my best to optimize this neck. I think `uniqueQueries` should be `Collections.synchronizedMap` to achieve LRU expiration, but `cache` does not need order and can be `ConcurrentHashMap` to improve performance.
doc count | field cardinality | query point | baseline QPS | candidate QPS | diff percentage | diff
-- | -- | -- | -- | -- | -- | --
30000000 | 10 | 1 | 2481 | 5102 | 105.6% |
30000000 | 10 | 1 | 2481 | 4843 | 95.2% | using `LongAdder `
30000000 | 10 | 1 | 2481 | 3751 | 51%| `uniqueQueries` and `cache` to be `Collections.synchronizedMap` .
30000000 | 10 | 1 | 2481 | 4571 | 84% | `uniqueQueries` to be`Collections.synchronizedMap` , `cache` to be`ConcurrentHashMap`
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579103250
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
can we adjust `LinkedHashMap ` to be `ConcurrentHashMap` ?
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1581809438
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
In my optimization, `uniqueQueries` is `Collections.synchronizedMap(LinkedHashMap)` to achieve LRU expiration, the writing to `cache` object is protected via the write lock, but reading `cache` object can be a concurrent action via making `cache` object to be `ConcurrentHashMap`. This is the key point of my optimization. I am also trying to read through all the concurrency paths.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1584475381
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -628,7 +632,7 @@ private class LeafCache implements Accountable {
LeafCache(Object key) {
this.key = key;
- cache = new IdentityHashMap<>();
+ cache = Collections.synchronizedMap(new IdentityHashMap<>());
Review Comment:
Sorry, I misunderstood before. Since there is a read lock, concurrent access to IdentityHashMap does not require additional synchronized lock. I optimized the code as per your suggestion.
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -628,7 +632,7 @@ private class LeafCache implements Accountable {
LeafCache(Object key) {
this.key = key;
- cache = new IdentityHashMap<>();
+ cache = Collections.synchronizedMap(new IdentityHashMap<>());
Review Comment:
Sorry, I misunderstood here before. Since there is a read lock, concurrent access to IdentityHashMap does not require additional synchronized lock. I optimized the code as per your suggestion.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1584480939
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -628,7 +632,7 @@ private class LeafCache implements Accountable {
LeafCache(Object key) {
this.key = key;
- cache = new IdentityHashMap<>();
+ cache = Collections.synchronizedMap(new IdentityHashMap<>());
Review Comment:
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579285322
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
In `ConcurrentHashMap`, although the order of insertion operations is uncertain, it ensures the thread safety of insertion operations. Multiple threads can perform insert operations simultaneously without causing data inconsistency or corruption. Still a 55% improvement.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579341971
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
How do you suggest that we achieve the "LRU" aspect of this cache without knowing the queries that were used most recently?
The point of the linked hashmap is that it orders by last access. We need something like this to achieve LRU expiration
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579596679
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
but I don't think it is the best to use `Collections.synchronizedMap`, we can use `ConcurrentLinkedDeque` to achieve LRU expiration better. Here is an example.
```
public class LRUCache<K, V> {
private final int capacity;
private ConcurrentMap<K, V> cache;
private ConcurrentLinkedDeque<K> keys;
public LRUCache(int capacity) {
this.capacity = capacity;
this.cache = new ConcurrentHashMap<>(capacity);
this.keys = new ConcurrentLinkedDeque<>();
}
public V get(K key) {
V value = cache.get(key);
if (value != null) {
keys.remove(key); // remove first occurrence of key
keys.addFirst(key); // add key to the head
}
return value;
}
public void put(K key, V value) {
if (cache.containsKey(key)) {
keys.remove(key); // remove first occurrence of key
}
while (keys.size() >= capacity) {
K lastKey = keys.removeLast();
if (lastKey != null) {
cache.remove(lastKey);
}
}
cache.put(key, value);
keys.addFirst(key);
}
}
```
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1580286445
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
Yes, So I want to try my best to optimize this neck. I think `uniqueQueries` should be `Collections.synchronizedMap` to achieve LRU expiration, but `cache` does not need order and can be `ConcurrentHashMap` to improve performance.
doc count | field cardinality | query point | baseline QPS | candidate QPS | diff percentage | diff
-- | -- | -- | -- | -- | -- | --
30000000 | 10 | 1 | 2481 | 5102 | 105.6% |
30000000 | 10 | 1 | 2481 | 4843 | 95.2% | using `LongAdder`
30000000 | 10 | 1 | 2481 | 3751 | 51%| `uniqueQueries` and `cache` to be `Collections.synchronizedMap`. using `LongAdder`
30000000 | 10 | 1 | 2481 | 4571 | 84% | `uniqueQueries` to be`Collections.synchronizedMap`, `cache` to be`ConcurrentHashMap`. using `LongAdder`
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1592821514
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -96,14 +98,15 @@ public class LRUQueryCache implements QueryCache, Accountable {
// mostRecentlyUsedQueries. This is why write operations are performed under a lock
private final Set<Query> mostRecentlyUsedQueries;
private final Map<IndexReader.CacheKey, LeafCache> cache;
- private final ReentrantLock lock;
+ private final ReentrantReadWriteLock.ReadLock readLock;
+ private final ReentrantReadWriteLock.WriteLock writeLock;
private final float skipCacheFactor;
// these variables are volatile so that we do not need to sync reads
// but increments need to be performed under the lock
private volatile long ramBytesUsed;
- private volatile long hitCount;
- private volatile long missCount;
+ private volatile LongAdder hitCount;
+ private volatile LongAdder missCount;
Review Comment:
These fields could be made final and non volative now.
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -911,15 +919,15 @@ public BulkScorer bulkScorer(LeafReaderContext context) throws IOException {
}
// If the lock is already busy, prefer using the uncached version than waiting
- if (lock.tryLock() == false) {
+ if (writeLock.tryLock() == false) {
Review Comment:
Why are we using the write lock here? It looks like we're only reading so we could use the read lock?
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1578328936
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
As with anything multi-threaded, this is tricky. How we are actually doing the LRU, is via the `uniqueQueries` which is a `LinkedHashMap` constructed by `new LinkedHashMap<>(16, 0.75f, true);`
Which means, a `get` against this will actually do an UPDATE to the LinkedHashMap order. If multiple readers attempt to do this at the same time, I would think this would then become indeterminate.
From the java docs to `LinkedHashMap`
```
* <p><strong>Note that this implementation is not synchronized.</strong>
* If multiple threads access a linked hash map concurrently, and at least
* one of the threads modifies the map structurally, it <em>must</em> be
* synchronized externally. This is typically accomplished by
* synchronizing on some object that naturally encapsulates the map.
```
Since we have order dictated at access, not on insertion, we are modifying the map structurally on read.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579285322
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
In `ConcurrentHashMap`, although the order of insertion operations is uncertain, it ensures the thread safety of insertion operations. Multiple threads can perform insert operations simultaneously without causing data inconsistency or corruption.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579596679
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
but I don't think it is the best to use `Collections.synchronizedMap`, we can use `ConcurrentLinkedDeque` to achieve LRU expiration with better performance. Here is an example.
```
public class LRUCache<K, V> {
private final int capacity;
private ConcurrentMap<K, V> cache;
private ConcurrentLinkedDeque<K> keys;
public LRUCache(int capacity) {
this.capacity = capacity;
this.cache = new ConcurrentHashMap<>(capacity);
this.keys = new ConcurrentLinkedDeque<>();
}
public V get(K key) {
V value = cache.get(key);
if (value != null) {
keys.remove(key); // remove first occurrence of key
keys.addFirst(key); // add key to the head
}
return value;
}
public void put(K key, V value) {
if (cache.containsKey(key)) {
keys.remove(key); // remove first occurrence of key
}
while (keys.size() >= capacity) {
K lastKey = keys.removeLast();
if (lastKey != null) {
cache.remove(lastKey);
}
}
cache.put(key, value);
keys.addFirst(key);
}
}
```
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1581231926
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
I agree, but IdentityHashMap is not thread-safe.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1580286445
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
Yes, So I want to try my best to optimize this neck. I think `uniqueQueries` should be `Collections.synchronizedMap` to achieve LRU expiration, but `cache` does not need order and can be `ConcurrentHashMap` to improve performance.
doc count | field cardinality | query point | baseline QPS | candidate QPS | diff percentage | diff
-- | -- | -- | -- | -- | -- | --
30000000 | 10 | 1 | 2481 | 5102 | 105.6% |
30000000 | 10 | 1 | 2481 | 4843 | 95.2% | `LongAdder `
30000000 | 10 | 1 | 2481 | 3751 | 51%| `LongAdder ` , `uniqueQueries` and `cache` to be `Collections.synchronizedMap` ,
30000000 | 10 | 1 | 2481 | 4571 | 84% | `LongAdder`, `uniqueQueries` to be`Collections.synchronizedMap` , `cache` to be`ConcurrentHashMap`
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1580286445
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
Yes, So I want to try my best to optimize this neck. I think `uniqueQueries` should be `Collections.synchronizedMap` to achieve LRU expiration, but `cache` does not need order and can be `ConcurrentHashMap` to improve performance.
doc count | field cardinality | query point | baseline QPS | candidate QPS | diff percentage | diff
-- | -- | -- | -- | -- | -- | --
30000000 | 10 | 1 | 2481 | 5102 | 105.6% |
30000000 | 10 | 1 | 2481 | 4843 | 95.2% | `LongAdder `
30000000 | 10 | 1 | 2481 | 3751 | 51%| `LongAdder ` , `uniqueQueries` to be `Collections.synchronizedMap` ,
30000000 | 10 | 1 | 2481 | 4571 | 84% | `LongAdder`, `uniqueQueries` to be`Collections.synchronizedMap` , `cache` to be`ConcurrentHashMap`
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579173947
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
I adjust `cache` AND `uniqueQueries` to be `ConcurrentHashMap` objects and get the benchmark results as follows.
doc count | field cardinality | query point | baseline QPS | candidate QPS | diff percentage
-- | -- | -- | -- | -- | --
30000000 | 10 | 1 | 2481 | 5102 | 105.6%
30000000 | 10 | 1 | 2481 | 4843 | 95.2% (using `LongAdder `)
30000000 | 10 | 1 | 2481 | 3851 | 55% (using `LongAdder ` And `ConcurrentHashMap`)
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on PR #13306:
URL: https://github.com/apache/lucene/pull/13306#issuecomment-2095147077
> @boicehuang what are the new numbers for your benchmarks for the current iteration? Indeed we will be synchronizing more, so I wonder if we will still see improvement.
doc count | field cardinality | query point | baseline QPS | candidate QPS | diff percentage | diff
-- | -- | -- | -- | -- | -- | --
30000000 | 10 | 1 | 2481 | 4428 | 78%| using `LongAdder`. `uniqueQueries` to be `Collections.synchronizedMap`. `cache` to be `IdentityHashMap`.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579596679
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
but I don't think it is the best to use `Collections.synchronizedMap`, we can use a better structure.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579103250
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
can we replace `LinkedHashMap ` with `ConcurrentHashMap` ?
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579769524
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
switching from an LRU to an LFU would require more consideration.
But, your debugging here shows that there is a fair amount of performance being left on the table. I am surprised to see how much of a bottle neck cache access actually is.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1574674149
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -177,7 +180,6 @@ public boolean test(LeafReaderContext context) {
* @lucene.experimental
*/
protected void onHit(Object readerCoreKey, Query query) {
- assert lock.isHeldByCurrentThread();
Review Comment:
It seems to me that the accuracy `hitCount` and `missCount` is now broken with this structure.
The volatile parameters can now be updated by more than one read-thread at a time, thus adding race conditions to their increments.
I do not think we want to add this level of uncertainty to cache statistics.
What happens to performance if you adjust `missCount` and `hitCount` to be `LongAdder` objects? This way the statistics are only locked when read, but still allows multiple lockless thread updates?
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1581809438
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
In my optimization, the writing to `cache` object was still protected via the write lock, but reading `cache` object concurrently via making `cache` object to be `ConcurrentHashMap`. This is the key point of my optimization. I
am also trying to read through all the concurrency paths with a picture.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1582091772
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
I drew 2 flow charts and tried to describe all the concurrency paths.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579285322
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
In `ConcurrentHashMap`, although the order of insertion operations is uncertain, which leads to test failure, it ensures the thread safety of insertion operations. Multiple threads can perform insert operations simultaneously without causing data inconsistency or corruption. It still has a 55% improvement.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579596679
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
but I don't think it is the best to use `Collections.synchronizedMap`, I wonder if we can use a better structure as below.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1580286445
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
Yes, So I want to try best to optimize this neck. I think `uniqueQueries` should be `Collections.synchronizedMap` to achieve LRU expiration, but `cache` can be `ConcurrentHashMap` to improve performance.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1580286445
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
Yes, So I want to try my best to optimize this neck. I think `uniqueQueries` should be `Collections.synchronizedMap` to achieve LRU expiration, but `cache` does not need order and can be `ConcurrentHashMap` to improve performance.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1581084470
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
I am currently trying to read through all the weird concurrency paths here. It's tricky to get correct.
`IdentityHashMap` is required still for the `Map<IndexReader.CacheKey, LeafCache>`. Changing to any other map will change behavior as now `IndexReader.CacheKey` isn't by object identity only, but object equality. This is a change to caching behavior.
`uniqueQueries` is used for the LRU portion of the cache, and the class key set is provided in `mostRecentlyUsedQueries`. Consequently, on `get` for `uniqueQueries`, that is a structural modification of the LinkedHashMap.
Then once we get the query, we look in `cache` with just a `get` on that map. This is NOT a structural modification for an IdentityHashMap.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1582091772
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
I think we still need `IdentityHashMap` to clear the cache of expired queries. I drew 2 flow charts and tried to describe all the concurrency paths of the newest modification.
--
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] Performance improvements to use RWLock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on PR #13306:
URL: https://github.com/apache/lucene/pull/13306#issuecomment-2103777811
> I think this is ready for merging. I can do the merging, but won't back port to 9x until we see nightlies. They might catch something we missed.
>
> @boicehuang could you add a changes entry to 9.11 under optimizations?
Do I need to open an issue at https://issues.apache.org/jira/projects/LUCENE/issues ? To get a Lucene-XXXXX number.
--
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] Performance improvements to use RWLock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1584480939
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -628,7 +632,7 @@ private class LeafCache implements Accountable {
LeafCache(Object key) {
this.key = key;
- cache = new IdentityHashMap<>();
+ cache = Collections.synchronizedMap(new IdentityHashMap<>());
Review Comment:
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1583351253
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -628,7 +632,7 @@ private class LeafCache implements Accountable {
LeafCache(Object key) {
this.key = key;
- cache = new IdentityHashMap<>();
+ cache = Collections.synchronizedMap(new IdentityHashMap<>());
Review Comment:
I don't understand this. Aren't all accesses to `LeafCache` protected by the LRUQueryCache read/write locks?
Since `LeafCache` isn't a static class, it should have access to the enclosing class's lock.
For testing safety, putIfAbsent, remove, onDocIdSetCache, and onDocIdSetEviction should all do a `assert writeLock.isHeldByCurrentThread();`
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1581809438
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
In my optimization, `uniqueQueries` is Collections.synchronizedMap to achieve LRU expiration, the writing to `cache` object was protected via the write lock, but reading `cache` object can be a concurrent action via making `cache` object to be `ConcurrentHashMap`. This is the key point of my optimization. I am also trying to read through all the concurrency paths.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1593322390
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -911,15 +919,15 @@ public BulkScorer bulkScorer(LeafReaderContext context) throws IOException {
}
// If the lock is already busy, prefer using the uncached version than waiting
- if (lock.tryLock() == false) {
+ if (writeLock.tryLock() == false) {
Review Comment:
Yes, read lock can also be used here
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -96,14 +98,15 @@ public class LRUQueryCache implements QueryCache, Accountable {
// mostRecentlyUsedQueries. This is why write operations are performed under a lock
private final Set<Query> mostRecentlyUsedQueries;
private final Map<IndexReader.CacheKey, LeafCache> cache;
- private final ReentrantLock lock;
+ private final ReentrantReadWriteLock.ReadLock readLock;
+ private final ReentrantReadWriteLock.WriteLock writeLock;
private final float skipCacheFactor;
// these variables are volatile so that we do not need to sync reads
// but increments need to be performed under the lock
private volatile long ramBytesUsed;
- private volatile long hitCount;
- private volatile long missCount;
+ private volatile LongAdder hitCount;
+ private volatile LongAdder missCount;
Review Comment:
ok
--
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] Performance improvements to use RWLock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on PR #13306:
URL: https://github.com/apache/lucene/pull/13306#issuecomment-2099953047
Current improvement number of the submitted code version.
doc count | field cardinality | query point | baseline QPS | candidate QPS | diff percentage | diff
-- | -- | -- | -- | -- | -- | --
30000000 | 10 | 1 | 2481 | 4408 | 78%| using `LongAdder`. `uniqueQueries` to be `Collections.synchronizedMap`. `cache` to be `IdentityHashMap`.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1580286445
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
Yes, So I want to try my best to optimize this neck. I think `uniqueQueries` should be `Collections.synchronizedMap` to achieve LRU expiration, but `cache` does not need order and can be `ConcurrentHashMap` to improve performance.
doc count | field cardinality | query point | baseline QPS | candidate QPS | diff percentage | diff
-- | -- | -- | -- | -- | -- | --
30000000 | 10 | 1 | 2481 | 5102 | 105.6% |
30000000 | 10 | 1 | 2481 | 4843 | 95.2% | using `LongAdder `
30000000 | 10 | 1 | 2481 | 3751 | 51%| `uniqueQueries` and `cache` to be `Collections.synchronizedMap`. using `LongAdder `
30000000 | 10 | 1 | 2481 | 4571 | 84% | `uniqueQueries` to be`Collections.synchronizedMap` , `cache` to be`ConcurrentHashMap`. using `LongAdder `
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579285322
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
In ConcurrentHashMap, although the order of insertion operations is uncertain, it ensures the thread safety of insertion operations. Multiple threads can perform insert operations simultaneously without causing data inconsistency or corruption.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579458103
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
I understand your point. I try to use `Collections.synchronizedMap` and get the benchmark results as follows
doc count | field cardinality | query point | baseline QPS | candidate QPS | diff percentage
-- | -- | -- | -- | -- | --
30000000 | 10 | 1 | 2481 | 5102 | 105.6%
30000000 | 10 | 1 | 2481 | 4843 | 95.2% (using `LongAdder `)
30000000 | 10 | 1 | 2481 | 3851 | 55% (using `LongAdder ` And `ConcurrentHashMap`)
30000000 | 10 | 1 | 2481 | 3751 | 51% (using `LongAdder ` And `Collections.synchronizedMap`)
It seems that performance can be improved if the time spent occupying the lock is minimized.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579596679
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
but I wonder if we can use a better structure as below.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1576153792
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -177,7 +180,6 @@ public boolean test(LeafReaderContext context) {
* @lucene.experimental
*/
protected void onHit(Object readerCoreKey, Query query) {
- assert lock.isHeldByCurrentThread();
Review Comment:
I adjust `missCount` and `hitCount` to be `LongAdder` objects and get the benchmark results as follows.
doc count | field cardinality | query point | baseline QPS | candidate QPS | diff percentage
-- | -- | -- | -- | -- | --
30000000 | 10 | 1 | 2481 | 5102 | 105.6%
30000000 | 10 | 1 | 2481 | 4843 | 95.2% (using `LongAdder `)
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1581809438
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
In my optimization, the writing to `cache` object was still protected via the write lock, but reading `cache` object can be a concurrent action via making `cache` object to be `ConcurrentHashMap`. This is the key point of my optimization. I am also trying to read through all the concurrency paths.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1580286445
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
Yes, So I want to try my best to optimize this neck. I think `uniqueQueries` should be `Collections.synchronizedMap` to achieve LRU expiration, but `cache` can be `ConcurrentHashMap` to improve performance.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1581481128
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
> I agree, but IdentityHashMap is not thread-safe.
I may be wrong, but I though if the writing to `IdentityHashMap` was protected (which it is via the write lock, which would block the readers), that means during read it isn't being mutated, and writes are fully synced. Consequently, its perfectly fine to read from multiple threads.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579596679
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
but I don't think it is the best to use `Collections.synchronizedMap`, we can use `ConcurrentLinkedDeque` to achieve LRU expiration with better performance. Here is an example.
```
public class LRUCache<K, V> {
private final int capacity;
private ConcurrentMap<K, V> cache;
private ConcurrentLinkedDeque<K> keys;
private final ReentrantReadWriteLock lock;
public LRUCache(int capacity) {
this.capacity = capacity;
this.cache = new ConcurrentHashMap<>(capacity);
this.keys = new ConcurrentLinkedDeque<>();
this.lock = new ReentrantReadWriteLock();
}
public V get(K key) {
lock.readLock().lock();
try {
V value = cache.get(key);
if (value != null) {
keys.remove(key); // remove first occurrence of key
keys.addFirst(key); // add key to the head
}
return value;
} finally {
lock.readLock().unlock();
}
}
public void put(K key, V value) {
lock.writeLock().lock();
try {
if (cache.containsKey(key)) {
keys.remove(key); // remove first occurrence of key
}
while (keys.size() >= capacity) {
K lastKey = keys.removeLast();
if (lastKey != null) {
cache.remove(lastKey);
}
}
cache.put(key, value);
keys.addFirst(key);
} finally {
lock.writeLock().unlock();
}
}
}
```
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579458103
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
I understand your point. I try to use `Collections.synchronizedMap` and get the benchmark results as follows
doc count | field cardinality | query point | baseline QPS | candidate QPS | diff percentage
-- | -- | -- | -- | -- | --
30000000 | 10 | 1 | 2481 | 5102 | 105.6%
30000000 | 10 | 1 | 2481 | 4843 | 95.2% (using `LongAdder `)
30000000 | 10 | 1 | 2481 | 3851 | 55% (using `LongAdder ` And `ConcurrentHashMap`)
30000000 | 10 | 1 | 2481 | 3751 | 51% (using `LongAdder ` And `Collections.synchronizedMap`)
It seems that performance still can be improved if the locking time is minimized.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1579173947
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
I adjust `cache` AND `uniqueQueries` to be `ConcurrentHashMap` objects and get the benchmark results as follows.
doc count | field cardinality | query point | baseline QPS | candidate QPS | diff percentage
-- | -- | -- | -- | -- | --
30000000 | 10 | 1 | 2481 | 5102 | 105.6%
30000000 | 10 | 1 | 2481 | 4843 | 95.2% (using `LongAdder `)
30000000 | 10 | 1 | 2481 | 3851 | 55% (using `LongAdder ` And `ConcurrentHashMap`)
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1583344572
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
The write lock will block the read lock (and read will block write). Meaning, structural changes to the `IdentityHashMap` are protected. It doesn't need to be wrapped `Collections.synchronizedMap`. In fact, the java docs for this lock has an example exactly like this but for TreeHashMap: https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html
--
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] Performance improvements to use RWLock to access LRUQueryCache [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent merged PR #13306:
URL: https://github.com/apache/lucene/pull/13306
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "boicehuang (via GitHub)" <gi...@apache.org>.
boicehuang commented on code in PR #13306:
URL: https://github.com/apache/lucene/pull/13306#discussion_r1582091772
##########
lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java:
##########
@@ -265,7 +269,6 @@ boolean requiresEviction() {
}
CacheAndCount get(Query key, IndexReader.CacheHelper cacheHelper) {
- assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
final IndexReader.CacheKey readerKey = cacheHelper.getKey();
Review Comment:
I think we still need `IdentityHashMap` to clear the cache of expired queries.
--
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] Performance improvements to use read lock to access LRUQueryCache [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #13306:
URL: https://github.com/apache/lucene/pull/13306#issuecomment-2088404582
@boicehuang what are the new numbers for your benchmarks for the current iteration? Indeed we will be synchronizing more, so I wonder if we will still see improvement.
--
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