You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/08/05 14:59:47 UTC
[GitHub] [lucene] jtibshirani opened a new pull request #232: LUCENE-10043: Decrease default LRUQuery#skipCacheFactor to 10
jtibshirani opened a new pull request #232:
URL: https://github.com/apache/lucene/pull/232
In LUCENE-9002 we introduced logic to skip caching a clause if it would be too
expensive compared to the usual query cost. Specifically, we avoid caching a
clause if its cost is estimated to be a 250x higher than the lead iterator's.
We've found that the default of 250 is quite high and can lead to poor tail
latencies. This PR decreases it to 10 to cache more conservatively.
--
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
[GitHub] [lucene] mayya-sharipova commented on a change in pull request #232: LUCENE-10043: Decrease default LRUQuery#skipCacheFactor to 10
Posted by GitBox <gi...@apache.org>.
mayya-sharipova commented on a change in pull request #232:
URL: https://github.com/apache/lucene/pull/232#discussion_r686130985
##########
File path: lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java
##########
@@ -146,7 +146,7 @@ public LRUQueryCache(
* of the top-level query will be cached in order to not hurt latency too much because of caching.
*/
public LRUQueryCache(int maxSize, long maxRamBytesUsed) {
- this(maxSize, maxRamBytesUsed, new MinSegmentSizePredicate(10000, .03f), 250);
+ this(maxSize, maxRamBytesUsed, new MinSegmentSizePredicate(10000, .03f), 10);
Review comment:
I've found [this comment](https://github.com/apache/lucene-solr/pull/940#discussion_r333896831) how `250` value for `skipCacheFactor` was translated into 10 times slower performance. But I guess how a query 's`cost` translates to a real performance cost depends on a query, and having `skipCacheFactor = 10` seems a safer option to me.
--
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
[GitHub] [lucene] jtibshirani commented on pull request #232: LUCENE-10043: Decrease default LRUQueryCache#skipCacheFactor to 10
Posted by GitBox <gi...@apache.org>.
jtibshirani commented on pull request #232:
URL: https://github.com/apache/lucene/pull/232#issuecomment-896706043
Thanks for the review.
--
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
[GitHub] [lucene] jtibshirani commented on a change in pull request #232: LUCENE-10043: Decrease default LRUQuery#skipCacheFactor to 10
Posted by GitBox <gi...@apache.org>.
jtibshirani commented on a change in pull request #232:
URL: https://github.com/apache/lucene/pull/232#discussion_r683543464
##########
File path: lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java
##########
@@ -146,7 +146,7 @@ public LRUQueryCache(
* of the top-level query will be cached in order to not hurt latency too much because of caching.
*/
public LRUQueryCache(int maxSize, long maxRamBytesUsed) {
- this(maxSize, maxRamBytesUsed, new MinSegmentSizePredicate(10000, .03f), 250);
+ this(maxSize, maxRamBytesUsed, new MinSegmentSizePredicate(10000, .03f), 10);
Review comment:
Working on this change made me wonder if `QueryCachingPolicy` should decide when to skip caching, as opposed to `LRUQueryCache`. In this case `skipCacheFactor` would move to `UsageTrackingQueryCachingPolicy`.
--
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
[GitHub] [lucene] mayya-sharipova commented on a change in pull request #232: LUCENE-10043: Decrease default LRUQuery#skipCacheFactor to 10
Posted by GitBox <gi...@apache.org>.
mayya-sharipova commented on a change in pull request #232:
URL: https://github.com/apache/lucene/pull/232#discussion_r686130368
##########
File path: lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java
##########
@@ -146,7 +146,7 @@ public LRUQueryCache(
* of the top-level query will be cached in order to not hurt latency too much because of caching.
*/
public LRUQueryCache(int maxSize, long maxRamBytesUsed) {
- this(maxSize, maxRamBytesUsed, new MinSegmentSizePredicate(10000, .03f), 250);
+ this(maxSize, maxRamBytesUsed, new MinSegmentSizePredicate(10000, .03f), 10);
Review comment:
It makes sense to me to move `skipCacheFactor` to `UsageTrackingQueryCachingPolicy`, as it this policy is now more than about usage.
--
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
[GitHub] [lucene] jtibshirani commented on a change in pull request #232: LUCENE-10043: Decrease default LRUQueryCache#skipCacheFactor to 10
Posted by GitBox <gi...@apache.org>.
jtibshirani commented on a change in pull request #232:
URL: https://github.com/apache/lucene/pull/232#discussion_r686703128
##########
File path: lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java
##########
@@ -146,7 +146,7 @@ public LRUQueryCache(
* of the top-level query will be cached in order to not hurt latency too much because of caching.
*/
public LRUQueryCache(int maxSize, long maxRamBytesUsed) {
- this(maxSize, maxRamBytesUsed, new MinSegmentSizePredicate(10000, .03f), 250);
+ this(maxSize, maxRamBytesUsed, new MinSegmentSizePredicate(10000, .03f), 10);
Review comment:
I plan to do this refactor in a follow-up on 9.0 only, to avoid changing the APIs too much on 8.x.
--
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
[GitHub] [lucene] jtibshirani commented on a change in pull request #232: LUCENE-10043: Decrease default LRUQueryCache#skipCacheFactor to 10
Posted by GitBox <gi...@apache.org>.
jtibshirani commented on a change in pull request #232:
URL: https://github.com/apache/lucene/pull/232#discussion_r686810436
##########
File path: lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java
##########
@@ -146,7 +146,7 @@ public LRUQueryCache(
* of the top-level query will be cached in order to not hurt latency too much because of caching.
*/
public LRUQueryCache(int maxSize, long maxRamBytesUsed) {
- this(maxSize, maxRamBytesUsed, new MinSegmentSizePredicate(10000, .03f), 250);
+ this(maxSize, maxRamBytesUsed, new MinSegmentSizePredicate(10000, .03f), 10);
Review comment:
I looked into this and now think the change isn't worth it -- it requires a new method on `QueryCachingPolicy` but doesn't help clarify or clean-up very much.
--
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
[GitHub] [lucene] jtibshirani merged pull request #232: LUCENE-10043: Decrease default LRUQueryCache#skipCacheFactor to 10
Posted by GitBox <gi...@apache.org>.
jtibshirani merged pull request #232:
URL: https://github.com/apache/lucene/pull/232
--
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