Bulk nextDocsAndScores for constant-score disjunction clauses - #16394
Bulk nextDocsAndScores for constant-score disjunction clauses#16394chengxis-mdb wants to merge 6 commits into
Conversation
MaxScoreBulkScorer's term-at-a-time scoring drains clauses through Scorer#nextDocsAndScores. TermScorer has a bulk implementation over postings, but ConstantScoreScorer falls back to one nextDoc() call per match. When the underlying iterator is a disjunction, every such call pays a DisiPriorityQueue update, and with tied constant scores the top-k threshold cannot prune, so the full candidate stream pays it. Drain a 4096-doc window in bulk via DocIdSetIterator#intoBitSet when the underlying iterator is a DisjunctionDISIApproximation; keep the doc-at-a-time loop for iterators that are cheap to advance and for two-phase iterators.
|
Benchmark results from luceneutil (wikimedium10m), run by my colleague Tianxiao Wei, including a new All other tasks are within noise (full run below). Note Full luceneutil outputWe also validated end-to-end on the production-like workload where we found the regression: top-k disjunctions of dense constant-score term-set clauses recover most of the slowdown introduced in 10.3 (avg latency 316ms → ~236ms across replicated runs), while equals/point-based and impact-scored shapes are unchanged. An earlier unconditional version of this patch (bulk path for every constant-score clause) measurably regressed single-postings and bit-set-backed clauses, which is what motivated the |
romseygeek
left a comment
romseygeek
left a comment
There was a problem hiding this comment.
Thanks for opening @chengxis-mdb! I left some comments.
| scoreMode == ScoreMode.TOP_SCORES ? new DocIdSetIteratorWrapper(disi) : disi; | ||
| this.twoPhaseIterator = null; | ||
| this.disi = this.approximation; | ||
| this.bulkDrainWorthwhile = disi instanceof DisjunctionDISIApproximation; |
There was a problem hiding this 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?
There was a problem hiding this 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, otherwiseFixedBitSet#intoArraysilently 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, matchingMaxScoreBulkScorer#INNER_WINDOW_SIZE. That is above the "8 and a couple hundreds" guidance onScorer#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 roughlywindow_size × density / num_sub_iteratorsheap 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
DisjunctionDISIApproximationoverridesintoArrayfor now, which keeps this change to the shape we actually measured.BitSetIteratorand the postings iterators could plausibly override it too, but that deserves its own benchmarking.
| 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 |
There was a problem hiding this 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.
There was a problem hiding this 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.
…tions The default implementation copies doc IDs one nextDoc() call at a time. DisjunctionDISIApproximation overrides it to load a window of doc IDs through #intoBitSet, which costs one bulk load and one priority-queue update per sub-iterator instead of one priority-queue update per matching doc.
…rray Iterators that load doc IDs in bulk now opt in by overriding #intoArray, which replaces the instanceof check on the underlying iterator. Also stop reporting an empty buffer while docs remain below upTo: callers read that as the iterator being exhausted, so a batch whose docs are all deleted used to drop every remaining hit of the clause.
|
Thanks @chengxis-mdb, this looks much better.
That would be great. |
The benchmark results from this luceneutil branch look good from the recent changes |
An empty buffer is documented to mean that no doc is left before upTo, and callers rely on it to terminate. Nothing enforced it, so an implementation that filters the buffer after loading it, e.g. on liveDocs, could report an empty buffer with docs still to come and silently drop hits. Assert it, and add a randomized ConstantScoreScorer test that reaches the case. Reaching it takes a doc ID range wider than one batch plus deletions that cluster, so the uniformly random deletions that existing randomized tests apply to small indexes never get there.
…ingScorer An empty buffer is documented to mean that no doc is left before upTo, and callers rely on it to terminate. Nothing enforced it, which is how the bug fixed in the previous commit went unnoticed. Assert it, and add a randomized test that reaches the case. Reaching it takes a doc ID range wider than one window plus deletions that cluster, so the uniformly random deletions that existing randomized tests apply to small indexes never get there: running the whole lucene:core suite against the unfixed code does not trip the assertion, only the targeted tests do. Backport of the same change on apache#16394.
3 participants
Description
#14701 rewrote
MaxScoreBulkScorerwindow scoring from doc-at-a-time to term-at-a-time, draining essential clauses throughScorer#nextDocsAndScores.TermScorergot a bulk implementation over postings (#14709), butConstantScoreScorerstill uses the default one-nextDoc()-per-match loop. When the constant-score clause wraps a disjunction (e.g. what aTermInSetQueryrewrites to below the boolean-rewrite threshold), everynextDoc()pays aDisiPriorityQueueupdate. With tied constant scores across clauses the top-k threshold stalls and cannot prune, so the entire candidate stream pays the heap maintenance — profiles of an affected workload show ~35% of CPU inDisiPriorityQueueN#updateTop/downHeap, all under the term-at-a-time paths.This change adds
DocIdSetIterator#intoArray(int upTo, int[] docs), mirroring the semantics of the existingDocIdStream#intoArray: copy doc IDs starting at the current one, stop beforeupToor when the array is full, and return0only when no doc ID belowupToremains. The default implementation is the doc-at-a-time loop.DisjunctionDISIApproximationoverrides it to load a window of doc IDs through#intoBitSetand flatten it withFixedBitSet#intoArray, which costs one bulk load and one heap update per sub-iterator instead of one heap update per matching doc — exactly the shape that regresses.ConstantScoreScorer#nextDocsAndScoresis then just a call tointoArray, so iterators opt into the bulk path by overriding the method rather than by a type check in the scorer. Two-phase iterators get the default doc-at-a-time implementation for free, sinceTwoPhaseIterator#asDocIdSetIteratordoes not overrideintoArray. OnlyDisjunctionDISIApproximationoverrides it here, which keeps the change scoped to the shape we measured; other iterators could plausibly benefit too, but that deserves its own benchmarking.The bulk window has to be bounded by the length of the target array, otherwise
FixedBitSet#intoArraywould silently truncate it. The batch size thatConstantScoreScorerasks for is therefore what caps the window, and it is set to 4096, matchingMaxScoreBulkScorer#INNER_WINDOW_SIZE. Note this also raises the batch size of the doc-at-a-time case, which used 64 before.Two correctness fixes come with it:
nextDocsAndScoresno longer reports an empty buffer while docs remain belowupTo. Callers such asMaxScoreBulkScorer#collectEssentialScoresIntoWindowloop onbuffer.size > 0, so a batch whose docs happened to be entirely deleted used to drop every remaining hit of the clause. It now loads another batch instead, the same wayTermScorer#nextDocsAndScoresdoes, andDisjunctionDISIApproximation#intoArraydoes the equivalent for an empty window so theintoArraycontract holds on its own. Covered byTestConstantScoreScorer#testNextDocsAndScoresSkipsFullyDeletedBatches.intoBitSetforwarding in the TOP_SCORESDocIdSetIteratorWrappergets a guard for the delegate swap done bysetMinCompetitiveScore, and the newintoArrayforwarding gets the same one: without it, the forwarding would call into an unpositioned empty iterator.The first of those two is a bug the buffer contract should have ruled out on its own:
Scorer#nextDocsAndScoresdocuments that an empty buffer means no doc is left beforeupTo, and every caller relies on it to terminate, but nothing checked it.AssertingScorernow asserts it, next to the existingintoBitSetpost-condition.To be clear about what that assertion buys, it does not surface the bug by itself: running all of
lucene:coreagainst the unfixed code (8536 tests,-Ptests.asserts=true) never trips it. Reaching a violation needs a doc ID range wider than one batch together with deletions clustered enough to wipe out a whole batch, and the uniformly random deletions that randomized tests apply to small indexes do not produce that.TestConstantScoreScorer#testNextDocsAndScoresRandomClusteredDeletionsbuilds that shape and drives the scorer throughAssertingScorer; it fails on the unfixed code both with assertions on (the new assert) and off (assertEquals). What the assertion buys is that any future violation fails at the violation instead of as a missing-hits mismatch somewhere downstream.Benchmark results (luceneutil, including a new constant-score disjunction task) are posted in the comments. Those numbers were measured on the earlier revision of this PR, which gated the bulk path on an
instanceofcheck and kept a batch of 64 for the doc-at-a-time case; a rerun on the current revision will be posted.