You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "original-brownbear (via GitHub)" <gi...@apache.org> on 2024/10/11 11:51:52 UTC
[PR] Dry up both the two ImpactsEnum Implementation ins Lucene912PostingsReader [lucene]
original-brownbear opened a new pull request, #13892:
URL: https://github.com/apache/lucene/pull/13892
These two share a lot of code, in particular the impacts implementation is 100% identical. We can save a lot of code and potentially some cycles for method invocations by drying things up. The changes are just mechanical field movements with the following exceptions:
1. One of the two implementations was using a bytes ref builder, one a bytes ref for holding the serialized impacts. The bytesref variant is faster so I used that for both when extracting.
2. Some simple arithmetic simplifications around the levels that should be obvious.
3. Removed the the logic for an index without positions in `BlockImpactsPostingsEnum`, that was dead code, we only set this thing up if there's positions.
--
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] Dry up both the two ImpactsEnum Implementation ins Lucene912PostingsReader [lucene]
Posted by "original-brownbear (via GitHub)" <gi...@apache.org>.
original-brownbear commented on PR #13892:
URL: https://github.com/apache/lucene/pull/13892#issuecomment-2408539235
All addressed Adrien or would you like me to split this up a little? :)
--
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] Dry up both the two ImpactsEnum Implementation ins Lucene912PostingsReader [lucene]
Posted by "original-brownbear (via GitHub)" <gi...@apache.org>.
original-brownbear commented on code in PR #13892:
URL: https://github.com/apache/lucene/pull/13892#discussion_r1797690836
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene912/Lucene912PostingsReader.java:
##########
@@ -1203,70 +1175,53 @@ public long cost() {
}
}
- final class BlockImpactsDocsEnum extends ImpactsEnum {
+ private abstract class BlockImpactsEnum extends ImpactsEnum {
- final ForDeltaUtil forDeltaUtil = new ForDeltaUtil();
- final PForUtil pforUtil = new PForUtil(new ForUtil());
+ protected final ForDeltaUtil forDeltaUtil = new ForDeltaUtil();
+ protected final PForUtil pforUtil = new PForUtil(new ForUtil());
- private final long[] docBuffer = new long[BLOCK_SIZE + 1];
- private final long[] freqBuffer = new long[BLOCK_SIZE];
+ protected final long[] docBuffer = new long[BLOCK_SIZE + 1];
+ protected final long[] freqBuffer = new long[BLOCK_SIZE];
- private int docBufferUpto;
+ protected final int docFreq; // number of docs in this posting list
- final IndexInput docIn;
- final PostingDecodingUtil docInUtil;
- final boolean indexHasFreq;
- final boolean indexHasPos;
+ protected final IndexInput docIn;
+ protected final PostingDecodingUtil docInUtil;
- private final int docFreq; // number of docs in this posting list
- private int docCountUpto; // number of docs in or before the current block
- private int doc; // doc we last read
- private long prevDocID; // last doc ID of the previous block
- private long freqFP;
+ protected int docCountUpto; // number of docs in or before the current block
+ protected int doc = -1; // doc we last read
+ protected long prevDocID = -1; // last doc ID of the previous block
+ protected int docBufferUpto = BLOCK_SIZE;
// true if we shallow-advanced to a new block that we have not decoded yet
- private boolean needsRefilling;
+ protected boolean needsRefilling;
// level 0 skip data
- private int level0LastDocID;
- private long level0DocEndFP;
- private final BytesRef level0SerializedImpacts;
- private final MutableImpactList level0Impacts;
+ protected int level0LastDocID = -1;
+ protected long level0DocEndFP;
+ protected final BytesRef level0SerializedImpacts;
+ protected final MutableImpactList level0Impacts;
// level 1 skip data
- private int level1LastDocID;
- private long level1DocEndFP;
- private int level1DocCountUpto;
- private final BytesRef level1SerializedImpacts;
- private final MutableImpactList level1Impacts;
-
- public BlockImpactsDocsEnum(FieldInfo fieldInfo, IntBlockTermState termState)
- throws IOException {
- indexHasFreq = fieldInfo.getIndexOptions().compareTo(IndexOptions.DOCS_AND_FREQS) >= 0;
- indexHasPos =
- fieldInfo.getIndexOptions().compareTo(IndexOptions.DOCS_AND_FREQS_AND_POSITIONS) >= 0;
- // We set the last element of docBuffer to NO_MORE_DOCS, it helps save conditionals in
- // advance()
- docBuffer[BLOCK_SIZE] = NO_MORE_DOCS;
-
- docFreq = termState.docFreq;
+ protected int level1LastDocID;
+ protected long level1DocEndFP;
+ protected int level1DocCountUpto = 0;
+ protected final BytesRef level1SerializedImpacts;
+ protected final MutableImpactList level1Impacts;
+
+ private BlockImpactsEnum(IntBlockTermState termState) throws IOException {
+ this.docFreq = termState.docFreq;
if (docFreq > 1) {
- docIn = Lucene912PostingsReader.this.docIn.clone();
- docInUtil = VECTORIZATION_PROVIDER.newPostingDecodingUtil(docIn);
+ this.docIn = Lucene912PostingsReader.this.docIn.clone();
+ this.docInUtil = VECTORIZATION_PROVIDER.newPostingDecodingUtil(docIn);
prefetchPostings(docIn, termState);
} else {
Review Comment:
Right, I guess it became more visible with this change that that's the case :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@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] Dry up both the two ImpactsEnum Implementation ins Lucene912PostingsReader [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #13892:
URL: https://github.com/apache/lucene/pull/13892#discussion_r1797607993
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene912/Lucene912PostingsReader.java:
##########
@@ -1203,70 +1175,53 @@ public long cost() {
}
}
- final class BlockImpactsDocsEnum extends ImpactsEnum {
+ private abstract class BlockImpactsEnum extends ImpactsEnum {
- final ForDeltaUtil forDeltaUtil = new ForDeltaUtil();
- final PForUtil pforUtil = new PForUtil(new ForUtil());
+ protected final ForDeltaUtil forDeltaUtil = new ForDeltaUtil();
+ protected final PForUtil pforUtil = new PForUtil(new ForUtil());
- private final long[] docBuffer = new long[BLOCK_SIZE + 1];
- private final long[] freqBuffer = new long[BLOCK_SIZE];
+ protected final long[] docBuffer = new long[BLOCK_SIZE + 1];
+ protected final long[] freqBuffer = new long[BLOCK_SIZE];
- private int docBufferUpto;
+ protected final int docFreq; // number of docs in this posting list
- final IndexInput docIn;
- final PostingDecodingUtil docInUtil;
- final boolean indexHasFreq;
- final boolean indexHasPos;
+ protected final IndexInput docIn;
+ protected final PostingDecodingUtil docInUtil;
- private final int docFreq; // number of docs in this posting list
- private int docCountUpto; // number of docs in or before the current block
- private int doc; // doc we last read
- private long prevDocID; // last doc ID of the previous block
- private long freqFP;
+ protected int docCountUpto; // number of docs in or before the current block
+ protected int doc = -1; // doc we last read
+ protected long prevDocID = -1; // last doc ID of the previous block
+ protected int docBufferUpto = BLOCK_SIZE;
// true if we shallow-advanced to a new block that we have not decoded yet
- private boolean needsRefilling;
+ protected boolean needsRefilling;
// level 0 skip data
- private int level0LastDocID;
- private long level0DocEndFP;
- private final BytesRef level0SerializedImpacts;
- private final MutableImpactList level0Impacts;
+ protected int level0LastDocID = -1;
+ protected long level0DocEndFP;
+ protected final BytesRef level0SerializedImpacts;
+ protected final MutableImpactList level0Impacts;
// level 1 skip data
- private int level1LastDocID;
- private long level1DocEndFP;
- private int level1DocCountUpto;
- private final BytesRef level1SerializedImpacts;
- private final MutableImpactList level1Impacts;
-
- public BlockImpactsDocsEnum(FieldInfo fieldInfo, IntBlockTermState termState)
- throws IOException {
- indexHasFreq = fieldInfo.getIndexOptions().compareTo(IndexOptions.DOCS_AND_FREQS) >= 0;
- indexHasPos =
- fieldInfo.getIndexOptions().compareTo(IndexOptions.DOCS_AND_FREQS_AND_POSITIONS) >= 0;
- // We set the last element of docBuffer to NO_MORE_DOCS, it helps save conditionals in
- // advance()
- docBuffer[BLOCK_SIZE] = NO_MORE_DOCS;
-
- docFreq = termState.docFreq;
+ protected int level1LastDocID;
+ protected long level1DocEndFP;
+ protected int level1DocCountUpto = 0;
+ protected final BytesRef level1SerializedImpacts;
+ protected final MutableImpactList level1Impacts;
+
+ private BlockImpactsEnum(IntBlockTermState termState) throws IOException {
+ this.docFreq = termState.docFreq;
if (docFreq > 1) {
- docIn = Lucene912PostingsReader.this.docIn.clone();
- docInUtil = VECTORIZATION_PROVIDER.newPostingDecodingUtil(docIn);
+ this.docIn = Lucene912PostingsReader.this.docIn.clone();
+ this.docInUtil = VECTORIZATION_PROVIDER.newPostingDecodingUtil(docIn);
prefetchPostings(docIn, termState);
} else {
Review Comment:
Not due to your change, but this else branch seems to be dead code since we use `SlowImpactsEnum` whenever `docFreq < BLOCK_SIZE`.
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene912/Lucene912PostingsReader.java:
##########
@@ -36,18 +36,9 @@
import org.apache.lucene.codecs.CodecUtil;
import org.apache.lucene.codecs.PostingsReaderBase;
import org.apache.lucene.codecs.lucene912.Lucene912PostingsFormat.IntBlockTermState;
-import org.apache.lucene.index.FieldInfo;
-import org.apache.lucene.index.Impact;
-import org.apache.lucene.index.Impacts;
-import org.apache.lucene.index.ImpactsEnum;
-import org.apache.lucene.index.IndexFileNames;
-import org.apache.lucene.index.IndexOptions;
-import org.apache.lucene.index.PostingsEnum;
-import org.apache.lucene.index.SegmentReadState;
-import org.apache.lucene.index.SlowImpactsEnum;
+import org.apache.lucene.index.*;
Review Comment:
Nit: We prefer to avoid star imports.
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene912/Lucene912PostingsReader.java:
##########
@@ -1326,15 +1347,12 @@ private void refillDocs() throws IOException {
if (left >= BLOCK_SIZE) {
forDeltaUtil.decodeAndPrefixSum(docInUtil, prevDocID, docBuffer);
-
- if (indexHasFreq) {
- freqFP = docIn.getFilePointer();
- PForUtil.skip(docIn);
- }
Review Comment:
Thanks for simplifying.
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene912/Lucene912PostingsReader.java:
##########
@@ -1316,8 +1257,88 @@ public BytesRef getPayload() {
}
@Override
- public int docID() {
- return doc;
+ public long cost() {
+ return docFreq;
+ }
+
+ private final Impacts impacts =
+ new Impacts() {
+
+ private final ByteArrayDataInput scratch = new ByteArrayDataInput();
+
+ @Override
+ public int numLevels() {
+ int numLevels = 0;
+ if (level0LastDocID != NO_MORE_DOCS) {
+ numLevels++;
+ }
+ if (level1LastDocID != NO_MORE_DOCS) {
+ numLevels++;
+ }
+ if (numLevels == 0) {
+ numLevels++;
+ }
+ return numLevels;
+ }
+
+ @Override
+ public int getDocIdUpTo(int level) {
+ if (level == 0) {
+ return level0LastDocID;
+ }
+ return level == 1 ? level1LastDocID : NO_MORE_DOCS;
+ }
+
+ @Override
+ public List<Impact> getImpacts(int level) {
+ if (level == 0 && level0LastDocID != NO_MORE_DOCS) {
+ return readImpacts(level0SerializedImpacts, level0Impacts);
+ }
+ if (level == 1 && level1LastDocID != NO_MORE_DOCS) {
+ return readImpacts(level1SerializedImpacts, level1Impacts);
+ }
+ return DUMMY_IMPACTS;
+ }
Review Comment:
I see you refactored this compared with how it's implemented in main. It looks simpler this way, and I believe it behaves the same since `level0LastDocID` being equal to NO_MORE_DOCS should imply `level1LastDocID` being equal to NO_MORE_DOCS too. Have you run luceneutil to check if we indeed return the same impacts, so performance shouldn't be affected?
--
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] Dry up both the two ImpactsEnum Implementation ins Lucene912PostingsReader [lucene]
Posted by "original-brownbear (via GitHub)" <gi...@apache.org>.
original-brownbear commented on PR #13892:
URL: https://github.com/apache/lucene/pull/13892#issuecomment-2408571620
Thanks Adrien!
--
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] Dry up both the two ImpactsEnum Implementation ins Lucene912PostingsReader [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #13892:
URL: https://github.com/apache/lucene/pull/13892#discussion_r1797694604
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene912/Lucene912PostingsReader.java:
##########
@@ -1326,8 +1270,88 @@ public BytesRef getPayload() {
}
@Override
- public int docID() {
- return doc;
+ public long cost() {
+ return docFreq;
+ }
+
+ private final Impacts impacts =
+ new Impacts() {
+
+ private final ByteArrayDataInput scratch = new ByteArrayDataInput();
+
+ @Override
+ public int numLevels() {
+ int numLevels = 0;
+ if (level0LastDocID != NO_MORE_DOCS) {
+ numLevels++;
+ }
+ if (level1LastDocID != NO_MORE_DOCS) {
+ numLevels++;
+ }
+ if (numLevels == 0) {
+ numLevels++;
+ }
+ return numLevels;
Review Comment:
Nit: this method could be simplified to `return level1LastDocID != NO_MORE_DOCS ? 2 : 1;`
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene912/Lucene912PostingsReader.java:
##########
@@ -1326,8 +1270,88 @@ public BytesRef getPayload() {
}
@Override
- public int docID() {
- return doc;
+ public long cost() {
+ return docFreq;
+ }
+
+ private final Impacts impacts =
+ new Impacts() {
+
+ private final ByteArrayDataInput scratch = new ByteArrayDataInput();
+
+ @Override
+ public int numLevels() {
+ int numLevels = 0;
+ if (level0LastDocID != NO_MORE_DOCS) {
+ numLevels++;
+ }
+ if (level1LastDocID != NO_MORE_DOCS) {
+ numLevels++;
+ }
+ if (numLevels == 0) {
+ numLevels++;
+ }
+ return numLevels;
+ }
+
+ @Override
+ public int getDocIdUpTo(int level) {
+ if (level == 0) {
+ return level0LastDocID;
+ }
+ return level == 1 ? level1LastDocID : NO_MORE_DOCS;
+ }
+
+ @Override
+ public List<Impact> getImpacts(int level) {
+ if (level == 0 && level0LastDocID != NO_MORE_DOCS) {
+ return readImpacts(level0SerializedImpacts, level0Impacts);
+ }
+ if (level == 1 && level1LastDocID != NO_MORE_DOCS) {
Review Comment:
Nit: The second condition is not necessary I believe (ie. it could be just `if (level == 1)`) as we only ever return 2 levels if `level1LastDocID != NO_MORE_DOCS`.
--
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] Dry up both the two ImpactsEnum Implementation ins Lucene912PostingsReader [lucene]
Posted by "original-brownbear (via GitHub)" <gi...@apache.org>.
original-brownbear commented on code in PR #13892:
URL: https://github.com/apache/lucene/pull/13892#discussion_r1797677131
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene912/Lucene912PostingsReader.java:
##########
@@ -1316,8 +1257,88 @@ public BytesRef getPayload() {
}
@Override
- public int docID() {
- return doc;
+ public long cost() {
+ return docFreq;
+ }
+
+ private final Impacts impacts =
+ new Impacts() {
+
+ private final ByteArrayDataInput scratch = new ByteArrayDataInput();
+
+ @Override
+ public int numLevels() {
+ int numLevels = 0;
+ if (level0LastDocID != NO_MORE_DOCS) {
+ numLevels++;
+ }
+ if (level1LastDocID != NO_MORE_DOCS) {
+ numLevels++;
+ }
+ if (numLevels == 0) {
+ numLevels++;
+ }
+ return numLevels;
+ }
+
+ @Override
+ public int getDocIdUpTo(int level) {
+ if (level == 0) {
+ return level0LastDocID;
+ }
+ return level == 1 ? level1LastDocID : NO_MORE_DOCS;
+ }
+
+ @Override
+ public List<Impact> getImpacts(int level) {
+ if (level == 0 && level0LastDocID != NO_MORE_DOCS) {
+ return readImpacts(level0SerializedImpacts, level0Impacts);
+ }
+ if (level == 1 && level1LastDocID != NO_MORE_DOCS) {
+ return readImpacts(level1SerializedImpacts, level1Impacts);
+ }
+ return DUMMY_IMPACTS;
+ }
Review Comment:
Jup ran luceneutil yesterday and and this version of things actually is slightly faster even :) (I guess because we have one fewer implementations of the impacts or some other minor effect from reducing codesize.
--
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] Dry up both the two ImpactsEnum Implementation ins Lucene912PostingsReader [lucene]
Posted by "original-brownbear (via GitHub)" <gi...@apache.org>.
original-brownbear commented on code in PR #13892:
URL: https://github.com/apache/lucene/pull/13892#discussion_r1797690775
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene912/Lucene912PostingsReader.java:
##########
@@ -36,18 +36,9 @@
import org.apache.lucene.codecs.CodecUtil;
import org.apache.lucene.codecs.PostingsReaderBase;
import org.apache.lucene.codecs.lucene912.Lucene912PostingsFormat.IntBlockTermState;
-import org.apache.lucene.index.FieldInfo;
-import org.apache.lucene.index.Impact;
-import org.apache.lucene.index.Impacts;
-import org.apache.lucene.index.ImpactsEnum;
-import org.apache.lucene.index.IndexFileNames;
-import org.apache.lucene.index.IndexOptions;
-import org.apache.lucene.index.PostingsEnum;
-import org.apache.lucene.index.SegmentReadState;
-import org.apache.lucene.index.SlowImpactsEnum;
+import org.apache.lucene.index.*;
Review Comment:
Ah right, I thought we're enforcing that here too but seems not :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@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] Dry up both the two ImpactsEnum Implementation ins Lucene912PostingsReader [lucene]
Posted by "original-brownbear (via GitHub)" <gi...@apache.org>.
original-brownbear merged PR #13892:
URL: https://github.com/apache/lucene/pull/13892
--
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