You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "easyice (via GitHub)" <gi...@apache.org> on 2024/01/07 14:15:29 UTC
[PR] Avoid reset BlockDocsEnum#freqBuffer when indexHasFreq is false [lucene]
easyice opened a new pull request, #12997:
URL: https://github.com/apache/lucene/pull/12997
As discussion in https://github.com/apache/lucene/pull/12954, this is the second implementation can be considered. this change yielded a 13% speedup for `PKLookup`.
--
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] Avoid reset BlockDocsEnum#freqBuffer when indexHasFreq is false [lucene]
Posted by "easyice (via GitHub)" <gi...@apache.org>.
easyice commented on code in PR #12997:
URL: https://github.com/apache/lucene/pull/12997#discussion_r1444747244
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99PostingsReader.java:
##########
@@ -399,7 +400,9 @@ public PostingsEnum reset(IntBlockTermState termState, int flags) throws IOExcep
doc = -1;
this.needsFreq = PostingsEnum.featureRequested(flags, PostingsEnum.FREQS);
this.isFreqsRead = true;
- if (indexHasFreq == false || needsFreq == false) {
+ // if reuse, we don't consider whether the needsFreq is changed. this is undefined in freq().
+ // see also: BasePostingsFormatTestCase#testPostingsEnumFreqs
+ if ((indexHasFreq == false || needsFreq == false) && reuse == false) {
Review Comment:
Thank you for explaining @jpountz! makes sense to me, the only problem is the `PostingsEnum#freq()` method might return 0, although this is undefined, is this looks good?
--
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] Avoid reset BlockDocsEnum#freqBuffer when indexHasFreq is false [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #12997:
URL: https://github.com/apache/lucene/pull/12997#discussion_r1444533812
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99PostingsReader.java:
##########
@@ -399,7 +400,9 @@ public PostingsEnum reset(IntBlockTermState termState, int flags) throws IOExcep
doc = -1;
this.needsFreq = PostingsEnum.featureRequested(flags, PostingsEnum.FREQS);
this.isFreqsRead = true;
- if (indexHasFreq == false || needsFreq == false) {
+ // if reuse, we don't consider whether the needsFreq is changed. this is undefined in freq().
+ // see also: BasePostingsFormatTestCase#testPostingsEnumFreqs
+ if ((indexHasFreq == false || needsFreq == false) && reuse == false) {
Review Comment:
Do we actually need to check `reuse` here? My understanding is that there are 3 cases:
1. Freqs exist and they are needed. In that case, refillDocs() will make sure to fill this array, so we don't need to do anything here.
2. Freqs are not needed. There is nothing to do as calling `PostingsEnum#freq()` is undefined.
3. Freqs are needed but they don't exist. In that case we need to initialize this array with ones here.
So it looks to me that we don't need to check the value of `reuse` and could change this condition to `if (indexHasFreq == false && needsFreqs)`? If you're saying this may be a bottleneck, we may also want to replace the for loop with a call to `Arrays#fill`.
--
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] Avoid reset BlockDocsEnum#freqBuffer when indexHasFreq is false [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz merged PR #12997:
URL: https://github.com/apache/lucene/pull/12997
--
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] Avoid reset BlockDocsEnum#freqBuffer when indexHasFreq is false [lucene]
Posted by "easyice (via GitHub)" <gi...@apache.org>.
easyice commented on PR #12997:
URL: https://github.com/apache/lucene/pull/12997#issuecomment-1882374457
> another approach could consist of filling at most docFreq entries in the freq buffer
It's a nice idea!
> I didn't know we had such leniency around frequencies/positions/offsets.
Yes, It feels like it should be more clear.
I found there is a big different speedup between java versions, the benchmark shows a significant speedup on java17, but only 3% on java21
**java17:**
```
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
PKLookup 89.14 (10.0%) 104.32 (9.1%) 17.0% ( -1% - 40%) 0.000
```
**java21:**
```
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
PKLookup 113.20 (11.9%) 116.58 (10.7%) 3.0% ( -17% - 28%) 0.402
```
--
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] Avoid reset BlockDocsEnum#freqBuffer when indexHasFreq is false [lucene]
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on PR #12997:
URL: https://github.com/apache/lucene/pull/12997#issuecomment-1883250347
> I tried to add checks to AssertingLeafReader to fail when reading freqs/positions/offsets/payloads if they have not been requested in the flags, and there are many test failures. It's not entirely clear to me how intentional that is, @rmuir do you have context by any chance?
@jpountz The current behavior is documented: https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/index/PostingsEnum.java#L73-L99
--
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] Avoid reset BlockDocsEnum#freqBuffer when indexHasFreq is false [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on PR #12997:
URL: https://github.com/apache/lucene/pull/12997#issuecomment-1881470555
Sorry I might have put you on the wrong track, I didn't know we had such leniency around frequencies/positions/offsets. Out of curiosity, I tried to add checks to AssertingLeafReader to fail when reading freqs/positions/offsets/payloads if they have not been requested in the flags, and there are many test failures. It's not entirely clear to me how intentional that is, @rmuir do you have context by any chance?
For the case you're trying to improve, another approach could consist of filling at most `docFreq` entries in the freq buffer:
```patch
diff --git a/lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99PostingsReader.java b/lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99PostingsReader.java
index 80d24917477..362fb34539e 100644
--- a/lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99PostingsReader.java
+++ b/lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99PostingsReader.java
@@ -400,9 +400,9 @@ public final class Lucene99PostingsReader extends PostingsReaderBase {
this.needsFreq = PostingsEnum.featureRequested(flags, PostingsEnum.FREQS);
this.isFreqsRead = true;
if (indexHasFreq == false || needsFreq == false) {
- for (int i = 0; i < ForUtil.BLOCK_SIZE; ++i) {
- freqBuffer[i] = 1;
- }
+ // Filling this buffer may not be cheap when doing primary key lookups, so we make sure to
+ // not fill more than `docFreq` entries.
+ Arrays.fill(freqBuffer, 0, Math.min(ForUtil.BLOCK_SIZE, docFreq), 1);
}
accum = 0;
blockUpto = 0;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Avoid reset BlockDocsEnum#freqBuffer when indexHasFreq is false [lucene]
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on PR #12997:
URL: https://github.com/apache/lucene/pull/12997#issuecomment-1883254450
as far as failing the methods, i'm not really sure if it is a good idea for frequencies. Treating the value as 1 seems reasonable, and e.g. i'm pretty sure TermScorer works correctly because of it, the `omitTF` has behaved like this for a really long time as far as i know, as the typical use case for this was super-short documents where frequency doesn't matter.
--
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