Build the AcceptDocs bit set from the raw filter iterator - #16588
Build the AcceptDocs bit set from the raw filter iterator#16588john-mlika wants to merge 1 commit into
Conversation
d8429e4 to
58674a3
Compare
| // Pass the raw iterator: #createBitSet applies liveDocs itself, and filtering upfront | ||
| // would hide DocIdSetIterator#intoBitSet behind a wrapper that has no bulk implementation. | ||
| DocIdSetIterator iterator = Objects.requireNonNull(iteratorSupplier.get()); |
There was a problem hiding this comment.
Did you consider implementing intoBitSet on the FilteredDocIdSetIterator object? I'm imagining something like the following in the getFilteredDocIdSetIterator method:
@Override
public void intoBitSet(int upTo, FixedBitSet bitSet, int offset) throws IOException {
getDelegate().intoBitSet(upTo, bitSet, offset);
liveDocs.applyMask(bitSet, offset);
}As it is, your solution improves performance when iterator.cost() >= threshold, but leaves the other branch without the benefit of the intoBitSet path.
There was a problem hiding this comment.
that was my first instinct, but the javadoc says intoBitSet must not clear bits that are already set, and delegating then applyMask masks the whole window, including bits a previous clause set. the accumulating callers share one destination, BooleanScorer ORs every clause iterator into the same window bitset, with a comment at the call site saying live docs get applied later. so a clause that clears bits erases its siblings' hits, and doing it safely needs a scratch bitset per window.
it also can't reach the branch you're pointing at: createBitSet only takes that path when cost < maxDoc >> 7, and BitSet.of picks a SparseFixedBitSet at that same cost, so intoBitSet, which only exists for FixedBitSet destinations, is never called there. that branch walks on the order of maxDoc/128 docs anyway.
There was a problem hiding this comment.
Thanks for the explanation, I hadn't read that part of the intoBitSet javadoc and missed the detail that only FixedBitSet uses the intoBitSet.
| // Pass the raw iterator: #createBitSet applies liveDocs itself, and filtering upfront | ||
| // would hide DocIdSetIterator#intoBitSet behind a wrapper that has no bulk implementation. | ||
| DocIdSetIterator iterator = Objects.requireNonNull(iteratorSupplier.get()); |
There was a problem hiding this comment.
Thanks for the explanation, I hadn't read that part of the intoBitSet javadoc and missed the detail that only FixedBitSet uses the intoBitSet.
58674a3 to
2a065d2
Compare
…nt (#157797) Without this override, we rely on the default implementation for intoBitSet, which is a for loop over the nextDoc() method. Now, we will instead call the wrapped method's intoBitSet method, which may be the default DocIdSetIterator#intoBitSet method, or may be a much more performant implementation. As a consequence, we may lose some insight into the number of nextDoc() calls performed, but in doing so, we gain a more accurate insight into the real runtime performance of a query's intoBitSet performance, which is an overall win for us. This is expected to improve profiled query performance by bringing it into closer alignment with runtime query performance. Note: there are some known cases in lucene where we rely on the default intoBitSet() implementation (iteratively calling `nextDoc()`). We expect a future upgrade of lucene will improve that performance. One example: apache/lucene#16588
2 participants
DocIdSetIteratorAcceptDocshandscreateBitSetan iterator already wrapped for live docs. The wrapper has nointoBitSet, so on any segment with deletions the bit set is filled onenextDoc()at a time and live docs are applied twice.createBitSetalready takesliveDocsinto account, so pass the raw iterator. Details and measurements are in #16586.Adds
TestAcceptDocs#testDenseIteratorIsConsumedInBulkWhenSegmentHasDeletions, which countsnextDoc()calls on the source iterator and fails on main when the segment has deletions and the accept set is dense enough for the bit-set branch, andtestRandomBitsAreMatchesIntersectedWithLiveDocs, which checks the bits are always the matches intersected with live docs on both sides of the dense/sparse threshold. Also addsFilteredKnnVectorQueryBenchmark, the benchmark the numbers in the issue come from.With the benchmark's 95%-selective cached filter and 5% deleted docs: 2.05 -> 1.23 ms/query.
Relates to #16586