romseygeek commented on code in PR #16394:
URL: https://github.com/apache/lucene/pull/16394#discussion_r3657554857
##########
lucene/core/src/java/org/apache/lucene/search/ConstantScoreScorer.java:
##########
@@ -86,6 +97,7 @@ public ConstantScoreScorer(float score, ScoreMode scoreMode,
DocIdSetIterator di
scoreMode == ScoreMode.TOP_SCORES ? new DocIdSetIteratorWrapper(disi)
: disi;
this.twoPhaseIterator = null;
this.disi = this.approximation;
+ this.bulkDrainWorthwhile = disi instanceof DisjunctionDISIApproximation;
Review Comment:
I don't think we want to be doing `instanceof` checks here. Can we instead
look at implementing an `intoArray()` method on `DocIdSetIterator`, with a
default implementation?
##########
lucene/core/src/java/org/apache/lucene/search/ConstantScoreScorer.java:
##########
@@ -154,18 +167,71 @@ public float score() throws IOException {
return score;
}
+ // Doc-ID window covered by one bulk #nextDocsAndScores fill. Matches
+ // MaxScoreBulkScorer.INNER_WINDOW_SIZE and
DenseConjunctionBulkScorer.WINDOW_SIZE, so a single
+ // fill covers a full inner scoring window with a bit set that stays
core-cache resident.
+ private static final int BULK_WINDOW_SIZE = 4096;
+
+ private FixedBitSet bulkWindowMatches; // lazily allocated
+
@Override
public void nextDocsAndScores(int upTo, Bits liveDocs,
DocAndFloatFeatureBuffer buffer)
throws IOException {
- int batchSize = 64;
- buffer.growNoCopy(batchSize);
- int size = 0;
+ if (bulkDrainWorthwhile == false) {
+ // Either matches must be verified one by one (two-phase), or the
iterator is cheap to
+ // advance one doc at a time (single postings list, bit set) and the
per-window fixed cost
+ // of the bulk path (bit set clear/flatten) would not pay for itself.
Only heap-based
+ // composite iterators (disjunctions), which pay a priority-queue update
per nextDoc(),
+ // benefit from the bulk drain.
+ int batchSize = 64;
+ buffer.growNoCopy(batchSize);
+ int size = 0;
+ DocIdSetIterator iterator = iterator();
+ for (int doc = iterator.docID(); doc < upTo && size < batchSize; doc =
iterator.nextDoc()) {
+ if (liveDocs == null || liveDocs.get(doc)) {
+ buffer.docs[size] = doc;
+ ++size;
+ }
+ }
+ Arrays.fill(buffer.features, 0, size, score);
+ buffer.size = size;
+ return;
+ }
+
+ // Drain a window of matches in bulk via DocIdSetIterator#intoBitSet.
Disjunctions implement
+ // it with one bulk load per sub-iterator, which is much cheaper than
paying a priority-queue
+ // update per nextDoc() call. This matters for constant-score disjunction
clauses under top-k
+ // scoring (MaxScoreBulkScorer), which have no impact-based bulk path.
+ buffer.size = 0;
DocIdSetIterator iterator = iterator();
- for (int doc = iterator.docID(); doc < upTo && size < batchSize; doc =
iterator.nextDoc()) {
- if (liveDocs == null || liveDocs.get(doc)) {
- buffer.docs[size] = doc;
- ++size;
+ int doc = iterator.docID();
+ if (doc >= upTo) {
+ return;
+ }
+ if (bulkWindowMatches == null) {
+ bulkWindowMatches = new FixedBitSet(BULK_WINDOW_SIZE);
+ } else {
+ bulkWindowMatches.clear();
+ }
+ int windowMax = (int) Math.min(upTo, (long) doc + BULK_WINDOW_SIZE);
+ iterator.intoBitSet(windowMax, bulkWindowMatches, doc);
+ int cardinality = bulkWindowMatches.cardinality();
+ if (cardinality == 0) {
+ // No match in this window; the iterator already advanced to windowMax
or beyond, the
Review Comment:
I don't think this behaviour is correct? If we return an empty
DocAndFloatFeatureBuffer then the caller will assume that the iterator is
exhausted.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
