chore: Seal BitSet to only allow FixedBitSet and SparseFixedBitSet#15161
chore: Seal BitSet to only allow FixedBitSet and SparseFixedBitSet#15161AdityaTeltia wants to merge 3 commits intoapache:mainfrom
Conversation
|
10.3 is cut, so cannot go there. Possibly 10.4. But I would expect this to be a v11 change. |
| * @lucene.internal | ||
| */ | ||
| public abstract class BitSet implements Bits, Accountable { | ||
| public abstract sealed class BitSet implements Bits, Accountable |
There was a problem hiding this comment.
Can you add a small comment explaining that we're doing this to help call sites of methods on the BitSet class be bimorphic at most, to help with performance?
There was a problem hiding this comment.
added small comment explaining this.
lucene/CHANGES.txt
Outdated
| (Ben Trent) | ||
|
|
||
| * GITHUB#15038: BitSet is now a sealed abstract class permitting only FixedBitSet and SparseFixedBitSet. | ||
|
|
| final int numBits = 1 + random().nextInt(100000); | ||
| for (float percentSet : new float[] {0, 0.01f, 0.1f, 0.5f, 0.9f, 0.99f, 1f}) { | ||
| BitSet set1 = new JavaUtilBitSet(randomSet(numBits, percentSet), numBits); | ||
| BitSet set1 = fromJavaUtilBitSet(randomSet(numBits, percentSet), numBits); |
There was a problem hiding this comment.
E.g. doing just java.util.BitSet set1 = randomSet(numBits, percentSet); should be good enough here since we're only comparing cardinalities?
| return randomSet(numBits, (int) (percentSet * numBits)); | ||
| } | ||
|
|
||
| protected abstract T fromJavaUtilBitSet(java.util.BitSet set, int numBits); |
There was a problem hiding this comment.
I worry that copying from a java.util.BitSet defeats the purpose of the test, since most tests would be comparing two same implementations of BitSet instead of JavaUtilBitSet vs. a BitSet implementation. Can we try to refactor tests to work directly against a java.util.BitSet without copying to a BitSet?
There was a problem hiding this comment.
tried refactoring it using java.util.Bitset.
Had to hack through two issues.
java.util.BitSet.nextSetBit()returns-1when there are no more set bits while lucene Bitset returnDocIdSetIterator.NO_MORE_DOCSi.e. 2147483647.java.util.BitSet.clear()throws error if from > to while lucene does nothing.
AdityaTeltia
force-pushed
the
seal-bitset-class
branch
from
54d2e50 to
472d630
Compare
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution! |
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution! |
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution! |
|
@jpountz @benwtrent Can we merge this now? |
3 participants
Closes #15038
correct BitSet implementation from a java.util.BitSet for testing.
This ensures that only Lucene’s optimized implementations of BitSet are allowed, preventing
accidental performance regressions from custom subclasses, while keeping existing tests valid.