From 8d195c86ded108b59a4c01cbc9905f131b8e129c Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Mon, 22 Jan 2018 12:43:34 +0100 Subject: [PATCH] CountedBitSet doesn't need to extend BitSet. (#28239) --- .../index/seqno/CountedBitSet.java | 36 +++---------------- .../index/seqno/LocalCheckpointTracker.java | 13 ++++--- .../index/translog/MultiSnapshot.java | 5 ++- .../index/seqno/CountedBitSetTests.java | 5 --- .../seqno/LocalCheckpointTrackerTests.java | 3 +- 5 files changed, 13 insertions(+), 49 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/seqno/CountedBitSet.java b/server/src/main/java/org/elasticsearch/index/seqno/CountedBitSet.java index 54270de1b01c8..d1f6f4a3a3745 100644 --- a/server/src/main/java/org/elasticsearch/index/seqno/CountedBitSet.java +++ b/server/src/main/java/org/elasticsearch/index/seqno/CountedBitSet.java @@ -19,7 +19,6 @@ package org.elasticsearch.index.seqno; -import org.apache.lucene.util.BitSet; import org.apache.lucene.util.FixedBitSet; import org.apache.lucene.util.RamUsageEstimator; @@ -28,7 +27,7 @@ * when all bits are set to reduce memory usage. This structure can work well for sequence numbers as * these numbers are likely to form contiguous ranges (eg. filling all bits). */ -public final class CountedBitSet extends BitSet { +public final class CountedBitSet { static final long BASE_RAM_BYTES_USED = RamUsageEstimator.shallowSizeOfInstance(CountedBitSet.class); private short onBits; // Number of bits are set. private FixedBitSet bitset; @@ -41,14 +40,12 @@ public CountedBitSet(short numBits) { this.bitset = new FixedBitSet(numBits); } - @Override public boolean get(int index) { assert 0 <= index && index < this.length(); assert bitset == null || onBits < bitset.length() : "Bitset should be released when all bits are set"; return bitset == null ? true : bitset.get(index); } - @Override public void set(int index) { assert 0 <= index && index < this.length(); assert bitset == null || onBits < bitset.length() : "Bitset should be released when all bits are set"; @@ -67,41 +64,16 @@ public void set(int index) { } } - @Override - public void clear(int startIndex, int endIndex) { - throw new UnsupportedOperationException(); - } - - @Override - public void clear(int index) { - throw new UnsupportedOperationException(); - } + // Below methods are pkg-private for testing - @Override - public int cardinality() { + int cardinality() { return onBits; } - @Override - public int length() { + int length() { return bitset == null ? onBits : bitset.length(); } - @Override - public int prevSetBit(int index) { - throw new UnsupportedOperationException(); - } - - @Override - public int nextSetBit(int index) { - throw new UnsupportedOperationException(); - } - - @Override - public long ramBytesUsed() { - return BASE_RAM_BYTES_USED + (bitset == null ? 0 : bitset.ramBytesUsed()); - } - boolean isInternalBitsetReleased() { return bitset == null; } diff --git a/server/src/main/java/org/elasticsearch/index/seqno/LocalCheckpointTracker.java b/server/src/main/java/org/elasticsearch/index/seqno/LocalCheckpointTracker.java index 34926a36f4573..cd33c1bf046ed 100644 --- a/server/src/main/java/org/elasticsearch/index/seqno/LocalCheckpointTracker.java +++ b/server/src/main/java/org/elasticsearch/index/seqno/LocalCheckpointTracker.java @@ -20,7 +20,6 @@ package org.elasticsearch.index.seqno; import com.carrotsearch.hppc.LongObjectHashMap; -import org.apache.lucene.util.BitSet; import org.elasticsearch.common.SuppressForbidden; /** @@ -39,7 +38,7 @@ public class LocalCheckpointTracker { * A collection of bit sets representing pending sequence numbers. Each sequence number is mapped to a bit set by dividing by the * bit set size. */ - final LongObjectHashMap processedSeqNo = new LongObjectHashMap<>(); + final LongObjectHashMap processedSeqNo = new LongObjectHashMap<>(); /** * The current local checkpoint, i.e., all sequence numbers no more than this number have been completed. @@ -96,7 +95,7 @@ public synchronized void markSeqNoAsCompleted(final long seqNo) { // this is possible during recovery where we might replay an operation that was also replicated return; } - final BitSet bitSet = getBitSetForSeqNo(seqNo); + final CountedBitSet bitSet = getBitSetForSeqNo(seqNo); final int offset = seqNoToBitSetOffset(seqNo); bitSet.set(offset); if (seqNo == checkpoint + 1) { @@ -170,7 +169,7 @@ assert getBitSetForSeqNo(checkpoint + 1).get(seqNoToBitSetOffset(checkpoint + 1) try { // keep it simple for now, get the checkpoint one by one; in the future we can optimize and read words long bitSetKey = getBitSetKey(checkpoint); - BitSet current = processedSeqNo.get(bitSetKey); + CountedBitSet current = processedSeqNo.get(bitSetKey); if (current == null) { // the bit set corresponding to the checkpoint has already been removed, set ourselves up for the next bit set assert checkpoint % BIT_SET_SIZE == BIT_SET_SIZE - 1; @@ -184,7 +183,7 @@ assert getBitSetForSeqNo(checkpoint + 1).get(seqNoToBitSetOffset(checkpoint + 1) */ if (checkpoint == lastSeqNoInBitSet(bitSetKey)) { assert current != null; - final BitSet removed = processedSeqNo.remove(bitSetKey); + final CountedBitSet removed = processedSeqNo.remove(bitSetKey); assert removed == current; current = processedSeqNo.get(++bitSetKey); } @@ -210,11 +209,11 @@ private long getBitSetKey(final long seqNo) { return seqNo / BIT_SET_SIZE; } - private BitSet getBitSetForSeqNo(final long seqNo) { + private CountedBitSet getBitSetForSeqNo(final long seqNo) { assert Thread.holdsLock(this); final long bitSetKey = getBitSetKey(seqNo); final int index = processedSeqNo.indexOf(bitSetKey); - final BitSet bitSet; + final CountedBitSet bitSet; if (processedSeqNo.indexExists(index)) { bitSet = processedSeqNo.indexGet(index); } else { diff --git a/server/src/main/java/org/elasticsearch/index/translog/MultiSnapshot.java b/server/src/main/java/org/elasticsearch/index/translog/MultiSnapshot.java index 910d71a51a0a7..7ea241958f87c 100644 --- a/server/src/main/java/org/elasticsearch/index/translog/MultiSnapshot.java +++ b/server/src/main/java/org/elasticsearch/index/translog/MultiSnapshot.java @@ -20,7 +20,6 @@ package org.elasticsearch.index.translog; import com.carrotsearch.hppc.LongObjectHashMap; -import org.apache.lucene.util.BitSet; import org.elasticsearch.index.seqno.CountedBitSet; import org.elasticsearch.index.seqno.SequenceNumbers; @@ -85,7 +84,7 @@ public void close() throws IOException { static final class SeqNoSet { static final short BIT_SET_SIZE = 1024; - private final LongObjectHashMap bitSets = new LongObjectHashMap<>(); + private final LongObjectHashMap bitSets = new LongObjectHashMap<>(); /** * Marks this sequence number and returns true if it is seen before. @@ -93,7 +92,7 @@ static final class SeqNoSet { boolean getAndSet(long value) { assert value >= 0; final long key = value / BIT_SET_SIZE; - BitSet bitset = bitSets.get(key); + CountedBitSet bitset = bitSets.get(key); if (bitset == null) { bitset = new CountedBitSet(BIT_SET_SIZE); bitSets.put(key, bitset); diff --git a/server/src/test/java/org/elasticsearch/index/seqno/CountedBitSetTests.java b/server/src/test/java/org/elasticsearch/index/seqno/CountedBitSetTests.java index b014f82740640..bc4f58034d1dc 100644 --- a/server/src/test/java/org/elasticsearch/index/seqno/CountedBitSetTests.java +++ b/server/src/test/java/org/elasticsearch/index/seqno/CountedBitSetTests.java @@ -26,9 +26,7 @@ import java.util.stream.Collectors; import java.util.stream.IntStream; -import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.lessThan; public class CountedBitSetTests extends ESTestCase { @@ -55,7 +53,6 @@ public void testReleaseInternalBitSet() { int numBits = (short) randomIntBetween(8, 4096); final CountedBitSet countedBitSet = new CountedBitSet((short) numBits); final List values = IntStream.range(0, numBits).boxed().collect(Collectors.toList()); - final long ramBytesUsedWithBitSet = countedBitSet.ramBytesUsed(); for (int i = 1; i < numBits; i++) { final int value = values.get(i); @@ -68,7 +65,6 @@ public void testReleaseInternalBitSet() { assertThat(countedBitSet.isInternalBitsetReleased(), equalTo(false)); assertThat(countedBitSet.length(), equalTo(numBits)); assertThat(countedBitSet.cardinality(), equalTo(i)); - assertThat(countedBitSet.ramBytesUsed(), equalTo(ramBytesUsedWithBitSet)); } // The missing piece to fill all bits. @@ -83,7 +79,6 @@ public void testReleaseInternalBitSet() { assertThat(countedBitSet.isInternalBitsetReleased(), equalTo(true)); assertThat(countedBitSet.length(), equalTo(numBits)); assertThat(countedBitSet.cardinality(), equalTo(numBits)); - assertThat(countedBitSet.ramBytesUsed(), allOf(equalTo(CountedBitSet.BASE_RAM_BYTES_USED), lessThan(ramBytesUsedWithBitSet))); } // Tests with released internal bitset. diff --git a/server/src/test/java/org/elasticsearch/index/seqno/LocalCheckpointTrackerTests.java b/server/src/test/java/org/elasticsearch/index/seqno/LocalCheckpointTrackerTests.java index 31b8c23bf1c79..932fb71790800 100644 --- a/server/src/test/java/org/elasticsearch/index/seqno/LocalCheckpointTrackerTests.java +++ b/server/src/test/java/org/elasticsearch/index/seqno/LocalCheckpointTrackerTests.java @@ -20,7 +20,6 @@ package org.elasticsearch.index.seqno; import com.carrotsearch.hppc.LongObjectHashMap; -import org.apache.lucene.util.BitSet; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.Randomness; import org.elasticsearch.common.util.concurrent.AbstractRunnable; @@ -260,7 +259,7 @@ public void testResetCheckpoint() { tracker.resetCheckpoint(localCheckpoint); assertThat(tracker.getCheckpoint(), equalTo((long) localCheckpoint)); assertThat(tracker.getMaxSeqNo(), equalTo((long) maxSeqNo)); - assertThat(tracker.processedSeqNo, new BaseMatcher>() { + assertThat(tracker.processedSeqNo, new BaseMatcher>() { @Override public boolean matches(Object item) { return (item instanceof LongObjectHashMap && ((LongObjectHashMap) item).isEmpty());