salvatore-campagna commented on code in PR #16623:
URL: https://github.com/apache/lucene/pull/16623#discussion_r4166709152
##########
lucene/core/src/java/org/apache/lucene/util/LiveDocs.java:
##########
@@ -82,4 +82,13 @@ public interface LiveDocs extends Bits {
* @return the number of deleted documents in this segment
*/
int deletedCount();
+
+ /**
+ * Materializes this LiveDocs as a {@link FixedBitSet} where set bits
represent live documents.
+ *
+ * <p>Used by {@link FixedBitSet#copyOf(Bits)} to avoid the per-bit generic
loop.
+ *
+ * @return a new {@link FixedBitSet} with set bits for live documents
+ */
+ FixedBitSet toFixedBitSet();
Review Comment:
I think this method and all its implementations are not necessary.
`SparseLiveDocs.toFixedBitSet` is already just `set(0, maxDoc)` plus
`applyMask(result, 0)`, which is generic, so `copyOf` can call
`bits.applyMask(bitSet, 0)` instead of the per bit loop. `bits` is already a
`Bits` there, so no `instanceof` and no new method, and dense gets `andRange`
for free. Right?
##########
lucene/core/src/java/org/apache/lucene/util/LiveDocs.java:
##########
@@ -82,4 +82,13 @@ public interface LiveDocs extends Bits {
* @return the number of deleted documents in this segment
*/
int deletedCount();
+
+ /**
Review Comment:
(if the method stays, see:
https://github.com/apache/lucene/pull/16623/changes#r4166709152)
Both implementations return a fresh bit set today, but to avoid a possible
future issue can we state in the Javadoc that the returned `FixedBitSet` is the
caller's to mutate and must not share state with this instance?
`PendingDeletes#getMutableBits` publishes what `copyOf` returns as the
segment's live docs and then clears bits in it, so an implementation returning
its backing set directly would mutate live docs shared with open readers. It is
a tempting shortcut in DenseLiveDocs in particular, since it already holds
exactly the right object.
##########
lucene/core/src/test/org/apache/lucene/util/TestLiveDocsCopyOf.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.util;
+
+import org.apache.lucene.tests.util.LuceneTestCase;
+
+/** Tests for {@link LiveDocs#toFixedBitSet()} via {@link
FixedBitSet#copyOf(Bits)}. */
+public class TestLiveDocsCopyOf extends LuceneTestCase {
+
+ public void testDenseLiveDocsCopyOf() {
+ int maxDoc = 1000;
+ FixedBitSet liveBits = new FixedBitSet(maxDoc);
+ liveBits.set(0, maxDoc);
+ // Delete some known positions
+ liveBits.clear(0);
+ liveBits.clear(42);
+ liveBits.clear(999);
+
+ DenseLiveDocs dense = DenseLiveDocs.builder(liveBits, maxDoc).build();
+ FixedBitSet copy = FixedBitSet.copyOf(dense);
+
+ assertEquals(maxDoc, copy.length());
+ for (int i = 0; i < maxDoc; i++) {
+ assertEquals("mismatch at doc " + i, dense.get(i), copy.get(i));
+ }
+ }
+
+ public void testSparseLiveDocsCopyOf() {
+ int maxDoc = 1000;
+ SparseFixedBitSet deletedDocs = new SparseFixedBitSet(maxDoc);
+ deletedDocs.set(0);
+ deletedDocs.set(42);
+ deletedDocs.set(999);
+
+ SparseLiveDocs sparse = SparseLiveDocs.builder(deletedDocs,
maxDoc).build();
+ FixedBitSet copy = FixedBitSet.copyOf(sparse);
+
+ assertEquals(maxDoc, copy.length());
+ for (int i = 0; i < maxDoc; i++) {
+ assertEquals("mismatch at doc " + i, sparse.get(i), copy.get(i));
+ }
+ }
+
+ public void testRandomized() {
+ for (int iter = 0; iter < 50; iter++) {
Review Comment:
Nit: could we use `atLeast(50)` here and `atLeast(1000)` for the fixed
`maxDoc` values, so nightly runs widen with `tests.multiplier`? Also `maxDoc =
1000` sits inside a single 4096 bit `SparseFixedBitSet` block, so
`andNotRange`'s first block and last block cases are only hit incidentally.
Boundary values like 1, 63, 64, 65, 4095, 4096, 4097 plus all deleted and
exactly one deleted would be worth pinning.
##########
lucene/core/src/java/org/apache/lucene/util/DenseLiveDocs.java:
##########
@@ -182,6 +182,11 @@ public int deletedCount() {
return deletedCount;
}
+ @Override
+ public FixedBitSet toFixedBitSet() {
+ return liveDocs.clone();
Review Comment:
`clone()` keeps `liveDocs.length()`, but this instance's `length()` is
maxDoc, and the constructor only asserts `liveDocs.length() >= maxDoc`.
I wrote a test to reproduce a possible issue that happens when the backing
bit set is longer than maxDoc:
```
public void testCopyOfPaddedDenseLiveDocsPreservesLength() {
int maxDoc = 100;
// DenseLiveDocs only requires liveDocs.length() >= maxDoc (see
constructor)
FixedBitSet backing = new FixedBitSet(256);
backing.set(0, maxDoc);
backing.clear(7);
DenseLiveDocs dense = DenseLiveDocs.builder(backing, maxDoc).build();
assertEquals(maxDoc, dense.length());
FixedBitSet copy = FixedBitSet.copyOf(dense);
assertEquals(dense.length(), copy.length());
assertEquals(dense.length() - dense.deletedCount(), copy.cardinality());
}
```
##########
lucene/benchmark-jmh/src/java/org/apache/lucene/benchmark/jmh/SoftDeletesReaderBenchmark.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.benchmark.jmh;
+
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.concurrent.TimeUnit;
+import org.apache.lucene.document.Document;
+import org.apache.lucene.document.Field;
+import org.apache.lucene.document.NumericDocValuesField;
+import org.apache.lucene.document.StringField;
+import org.apache.lucene.index.DirectoryReader;
+import org.apache.lucene.index.IndexWriter;
+import org.apache.lucene.index.IndexWriterConfig;
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.index.SoftDeletesDirectoryReaderWrapper;
+import org.apache.lucene.index.Term;
+import org.apache.lucene.store.Directory;
+import org.apache.lucene.store.MMapDirectory;
+import org.apache.lucene.util.Bits;
+import org.apache.lucene.util.IOUtils;
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.OutputTimeUnit;
+import org.openjdk.jmh.annotations.Param;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.TearDown;
+import org.openjdk.jmh.annotations.Warmup;
+
+/**
+ * Measures the cost of wrapping a reader with {@link
SoftDeletesDirectoryReaderWrapper}, which
+ * calls {@link org.apache.lucene.util.FixedBitSet#copyOf(Bits)} on each
leaf's live docs. Since
+ * #15413, live docs are {@link org.apache.lucene.util.DenseLiveDocs} or {@link
+ * org.apache.lucene.util.SparseLiveDocs} which fall through to the per-bit
generic loop.
+ */
+@BenchmarkMode(Mode.AverageTime)
+@OutputTimeUnit(TimeUnit.MICROSECONDS)
+@State(Scope.Benchmark)
+@Warmup(iterations = 3, time = 2, timeUnit = TimeUnit.SECONDS)
+@Measurement(iterations = 5, time = 3, timeUnit = TimeUnit.SECONDS)
+@Fork(value = 3, jvmArgsPrepend = "--add-modules=jdk.incubator.vector")
+public class SoftDeletesReaderBenchmark {
+
+ private static final String SOFT_DELETE_FIELD = "__soft_delete";
+
+ @Param({"200000", "1000000"})
+ int numDocs;
+
+ @Param({"0.01", "0.05"})
+ double softDeleteRate;
+
+ private Directory dir;
+ private DirectoryReader baseReader;
+ private Path tempDir;
+
+ @Setup(Level.Trial)
+ public void setup() throws Exception {
+ tempDir = Path.of(System.getProperty("java.io.tmpdir"), "softdel-bench-" +
System.nanoTime());
+ dir = MMapDirectory.open(tempDir);
+
+ IndexWriterConfig config =
+ new
IndexWriterConfig().setSoftDeletesField(SOFT_DELETE_FIELD).setRAMBufferSizeMB(256);
+ try (IndexWriter w = new IndexWriter(dir, config)) {
+ for (int i = 0; i < numDocs; i++) {
+ Document doc = new Document();
+ doc.add(new StringField("id", Integer.toString(i), Field.Store.NO));
+ doc.add(new NumericDocValuesField("val", i));
+ w.addDocument(doc);
+ }
+ w.flush();
+ int softDeleted = (int) (numDocs * softDeleteRate);
+ for (int i = 0; i < softDeleted; i++) {
+ int docId = (int) (((long) i * 7919) % numDocs);
+ Document replacement = new Document();
+ replacement.add(new StringField("id", Integer.toString(docId),
Field.Store.NO));
+ replacement.add(new NumericDocValuesField("val", -1));
+ w.softUpdateDocument(
+ new Term("id", Integer.toString(docId)),
+ replacement,
+ new NumericDocValuesField(SOFT_DELETE_FIELD, 1));
+ }
+ int hardDeleted = Math.max(1, (int) (numDocs * 0.01));
Review Comment:
In `Lucene90LiveDocsFormat.readLiveDocs` the choice of representation
depends on hard deletes only: we call `info.getDelCount()`, which does not
include soft deletes, and compare `deletionRate` against
`SPARSE_DENSE_THRESHOLD = 0.01`.
Here the hard delete rate is hardcoded at `numDocs * 0.01`, right on that
threshold, and `softDeleteRate` cannot move it, so the deletion rate stays just
under 0.01 and `SparseLiveDocs` is selected. I get that `softDeleteRate` varies
how much of the wrap cost is `copyOf` versus `applySoftDeletes`, but
`DenseLiveDocs` never gets exercised end to end, and with `TieredMergePolicy`
allowing 20% deletes by default that is the common representation. Could we
make the hard delete rate a `@Param` with a value above 0.01 so that we see the
effect for both representations?
##########
lucene/core/src/test/org/apache/lucene/util/TestLiveDocsCopyOf.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.util;
+
+import org.apache.lucene.tests.util.LuceneTestCase;
+
+/** Tests for {@link LiveDocs#toFixedBitSet()} via {@link
FixedBitSet#copyOf(Bits)}. */
+public class TestLiveDocsCopyOf extends LuceneTestCase {
+
+ public void testDenseLiveDocsCopyOf() {
+ int maxDoc = 1000;
+ FixedBitSet liveBits = new FixedBitSet(maxDoc);
+ liveBits.set(0, maxDoc);
+ // Delete some known positions
+ liveBits.clear(0);
+ liveBits.clear(42);
+ liveBits.clear(999);
+
+ DenseLiveDocs dense = DenseLiveDocs.builder(liveBits, maxDoc).build();
+ FixedBitSet copy = FixedBitSet.copyOf(dense);
+
+ assertEquals(maxDoc, copy.length());
+ for (int i = 0; i < maxDoc; i++) {
+ assertEquals("mismatch at doc " + i, dense.get(i), copy.get(i));
+ }
+ }
+
+ public void testSparseLiveDocsCopyOf() {
+ int maxDoc = 1000;
+ SparseFixedBitSet deletedDocs = new SparseFixedBitSet(maxDoc);
+ deletedDocs.set(0);
+ deletedDocs.set(42);
+ deletedDocs.set(999);
+
+ SparseLiveDocs sparse = SparseLiveDocs.builder(deletedDocs,
maxDoc).build();
+ FixedBitSet copy = FixedBitSet.copyOf(sparse);
+
+ assertEquals(maxDoc, copy.length());
+ for (int i = 0; i < maxDoc; i++) {
+ assertEquals("mismatch at doc " + i, sparse.get(i), copy.get(i));
+ }
+ }
+
+ public void testRandomized() {
+ for (int iter = 0; iter < 50; iter++) {
+ int maxDoc = random().nextInt(10_000) + 1;
+ double deletionRate = random().nextDouble() * 0.5;
+ int numDeleted = (int) (maxDoc * deletionRate);
+
+ // Build both representations
+ FixedBitSet liveBits = new FixedBitSet(maxDoc);
+ liveBits.set(0, maxDoc);
+ SparseFixedBitSet deletedDocs = new SparseFixedBitSet(maxDoc);
+
+ for (int i = 0; i < numDeleted; i++) {
+ int docId = random().nextInt(maxDoc);
+ liveBits.clear(docId);
+ deletedDocs.set(docId);
+ }
+
+ DenseLiveDocs dense = DenseLiveDocs.builder(liveBits, maxDoc).build();
+ SparseLiveDocs sparse = SparseLiveDocs.builder(deletedDocs,
maxDoc).build();
+
+ // Build per-bit reference
+ FixedBitSet reference = new FixedBitSet(maxDoc);
+ for (int i = 0; i < maxDoc; i++) {
+ if (dense.get(i)) {
Review Comment:
Nit: the reference comes from `dense.get(i)`, so a bug in
`DenseLiveDocs.get()` would make both assertions agree with each other and
pass. Building it from the deleted doc ids instead would make it independent,
right?
##########
lucene/core/src/test/org/apache/lucene/util/TestLiveDocsCopyOf.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.util;
+
+import org.apache.lucene.tests.util.LuceneTestCase;
+
+/** Tests for {@link LiveDocs#toFixedBitSet()} via {@link
FixedBitSet#copyOf(Bits)}. */
+public class TestLiveDocsCopyOf extends LuceneTestCase {
+
+ public void testDenseLiveDocsCopyOf() {
+ int maxDoc = 1000;
+ FixedBitSet liveBits = new FixedBitSet(maxDoc);
+ liveBits.set(0, maxDoc);
+ // Delete some known positions
+ liveBits.clear(0);
+ liveBits.clear(42);
+ liveBits.clear(999);
+
+ DenseLiveDocs dense = DenseLiveDocs.builder(liveBits, maxDoc).build();
+ FixedBitSet copy = FixedBitSet.copyOf(dense);
Review Comment:
All three tests check content but none checks that the copy is detached from
the source, which is what `copyOf` is for: `PendingDeletes#getMutableBits`
mutates the result and publishes it as the segment's live docs. Could we add
something like `copy.clear(100); assertTrue(dense.get(100));` plus the same for
sparse? Today it would catch a change from `liveDocs.clone()` to return
`liveDocs;`, which otherwise leaves the suite green.
Related to comment:
https://github.com/apache/lucene/pull/16623/changes#r4166959798
##########
lucene/test-framework/src/java/org/apache/lucene/tests/index/AssertingLeafReader.java:
##########
@@ -1959,5 +1959,10 @@ public DocIdSetIterator liveDocsIterator() {
public DocIdSetIterator deletedDocsIterator() {
return liveDocs.deletedDocsIterator();
}
+
+ @Override
+ public FixedBitSet toFixedBitSet() {
Review Comment:
This override does not assert anything, unlike the others here.
`getLiveDocs()` above already asserts `maxDoc() == liveDocs.length()` for the
source. Maybe we can assert the same for the result here? Also, it would catch
the padded case I mentioned on `DenseLiveDocs`. Delegating also means `copyOf`
no longer goes through the asserting `get()`, so it loses the `assertThread`
check it used to get. Same for AssertingLiveDocsFormat.
--
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]
