chengxis-mdb commented on code in PR #16394:
URL: https://github.com/apache/lucene/pull/16394#discussion_r3658354127
##########
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:
Agreed, the `instanceof` was the weak part of this. Done in 38dea2f and
06273be.
`DocIdSetIterator#intoArray(int upTo, int[] docs)` now mirrors
`DocIdStream#intoArray`: the default implementation is the doc-at-a-time loop,
`DisjunctionDISIApproximation` overrides it to load a window through
`#intoBitSet`, and `ConstantScoreScorer#nextDocsAndScores` just calls it — no
`instanceof`, no gate, and the two-phase case gets the default implementation
for free since `TwoPhaseIterator#asDocIdSetIterator` doesn't override it.
`ConstantScoreScorer.DocIdSetIteratorWrapper` forwards `intoArray` to its
delegate (with the same guard as `intoBitSet`) so TOP_SCORES clauses still
reach the bulk implementation.
Two things worth flagging:
* The window has to be bounded by `docs.length`, otherwise
`FixedBitSet#intoArray` silently truncates it and we lose docs. So the batch
size the scorer asks for is what caps the window, and I set it to 4096,
matching `MaxScoreBulkScorer#INNER_WINDOW_SIZE`. That is above the "8 and a
couple hundreds" guidance on `Scorer#nextDocsAndScores`, and it now also
applies to the doc-at-a-time case, which used a batch of 64 before. I'm happy
to lower it if you'd rather keep the buffers small, but it isn't neutral for
the workload that motivated this: the saving is roughly `window_size × density
/ num_sub_iterators` heap operations, and at the ~4% per-clause density we see,
512 is close to break-even while 4096 is a ~16x reduction. I'll re-run
luceneutil on this shape and post fresh numbers either way.
* Only `DisjunctionDISIApproximation` overrides `intoArray` for now, which
keeps this change to the shape we actually measured. `BitSetIterator` and the
postings iterators could plausibly override it too, but that deserves its own
benchmarking.
##########
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:
You're right, and the bug is worse than the case I wrote that comment for.
Thanks for catching it.
The `cardinality == 0` branch itself turned out to be unreachable:
`nextDocsAndScores` starts on the current doc, the current doc is always inside
the window, so `intoBitSet` always sets at least one bit. But the `liveDocs`
filter right below it could empty the buffer while docs remained below `upTo`,
and callers such as `MaxScoreBulkScorer#collectEssentialScoresIntoWindow` and
`#scoreInnerWindowSingleEssentialClause` loop on `buffer.size > 0`. So a clause
whose current batch happened to be entirely deleted dropped all of its
remaining hits.
Fixed in 06273be: `nextDocsAndScores` now loops until a batch has at least
one live doc or the iterator has nothing left below `upTo`, the same shape as
`TermScorer#nextDocsAndScores`. `DisjunctionDISIApproximation#intoArray` does
the equivalent for an empty window, so the `intoArray` contract ("never return
0 while doc IDs below `upTo` remain") holds independently of the caller.
Regression test in
`TestConstantScoreScorer#testNextDocsAndScoresSkipsFullyDeletedBatches`: 10k
docs of which only the last 1000 are live, drained through the same loop shape
the bulk scorers use, for both `COMPLETE` and `TOP_SCORES` and both a plain and
a disjunction-backed iterator. Against the previous commit the disjunction case
returns `[]` on the very first call — all 1000 matching live docs lost — and it
now returns all of them.
--
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]
