Skip to content

Commit

Permalink
CountedBitSet doesn't need to extend BitSet. (#28239)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jpountz
jpountz authored Jan 22, 2018
1 parent 452c36c commit 8d195c8
Show file tree
Hide file tree
Showing 5 changed files with 13 additions and 49 deletions.
36 changes: 4 additions & 32 deletions server/src/main/java/org/elasticsearch/index/seqno/CountedBitSet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
Expand All @@ -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";
Expand All @@ -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;
}
Expand Down
13 changes: 6 additions & 7 deletions server/src/main/java/org/elasticsearch/index/seqno/LocalCheckpointTracker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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<BitSet> processedSeqNo = new LongObjectHashMap<>();
final LongObjectHashMap<CountedBitSet> processedSeqNo = new LongObjectHashMap<>();

/**
* The current local checkpoint, i.e., all sequence numbers no more than this number have been completed.
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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);
}
Expand All @@ -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 {
Expand Down
5 changes: 2 additions & 3 deletions server/src/main/java/org/elasticsearch/index/translog/MultiSnapshot.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -85,15 +84,15 @@ public void close() throws IOException {

static final class SeqNoSet {
static final short BIT_SET_SIZE = 1024;
private final LongObjectHashMap<BitSet> bitSets = new LongObjectHashMap<>();
private final LongObjectHashMap<CountedBitSet> bitSets = new LongObjectHashMap<>();

/**
* Marks this sequence number and returns <tt>true</tt> if it is seen before.
*/
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);
Expand Down
5 changes: 0 additions & 5 deletions server/src/test/java/org/elasticsearch/index/seqno/CountedBitSetTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand All @@ -55,7 +53,6 @@ public void testReleaseInternalBitSet() {
int numBits = (short) randomIntBetween(8, 4096);
final CountedBitSet countedBitSet = new CountedBitSet((short) numBits);
final List<Integer> 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);
Expand All @@ -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.
Expand All @@ -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.
Expand Down
3 changes: 1 addition & 2 deletions server/src/test/java/org/elasticsearch/index/seqno/LocalCheckpointTrackerTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<LongObjectHashMap<BitSet>>() {
assertThat(tracker.processedSeqNo, new BaseMatcher<LongObjectHashMap<CountedBitSet>>() {
@Override
public boolean matches(Object item) {
return (item instanceof LongObjectHashMap && ((LongObjectHashMap) item).isEmpty());
Expand Down

0 comments on commit 8d195c8

Please sign in to comment.