From 3c6deb22bb8793fea21635ceb3f45994c2fdf6fb Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Sun, 3 Dec 2017 15:47:52 -0500 Subject: [PATCH 1/3] Tighten the CountedBitSet class This commit addresses the missed comments from https://github.com/elastic/elasticsearch/pull/27547. --- .../index/translog/CountedBitSet.java | 20 ++++++++++--------- .../index/translog/CountedBitSetTests.java | 4 ++++ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/translog/CountedBitSet.java b/core/src/main/java/org/elasticsearch/index/translog/CountedBitSet.java index 9fac230c9a8f2..a2ae3970a39a9 100644 --- a/core/src/main/java/org/elasticsearch/index/translog/CountedBitSet.java +++ b/core/src/main/java/org/elasticsearch/index/translog/CountedBitSet.java @@ -21,6 +21,7 @@ import org.apache.lucene.util.BitSet; import org.apache.lucene.util.FixedBitSet; +import org.apache.lucene.util.RamUsageEstimator; /** * A {@link CountedBitSet} wraps a {@link FixedBitSet} but automatically releases the internal bitset @@ -28,11 +29,14 @@ * from translog as these numbers are likely to form contiguous ranges (eg. filling all bits). */ final class CountedBitSet extends BitSet { + private static final long BASE_RAM_BYTES_USED = RamUsageEstimator.shallowSizeOfInstance(CountedBitSet.class); private short onBits; // Number of bits are set. private FixedBitSet bitset; CountedBitSet(short numBits) { - assert numBits > 0; + if (numBits <= 0) { + throw new IllegalArgumentException("Number of bits must be positive. Given [" + numBits + "]"); + } this.onBits = 0; this.bitset = new FixedBitSet(numBits); } @@ -41,7 +45,6 @@ final class CountedBitSet extends BitSet { 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); } @@ -52,7 +55,7 @@ public void set(int index) { // Ignore set when bitset is full. if (bitset != null) { - boolean wasOn = bitset.getAndSet(index); + final boolean wasOn = bitset.getAndSet(index); if (wasOn == false) { onBits++; // Once all bits are set, we can simply just return YES for all indexes. @@ -66,12 +69,12 @@ public void set(int index) { @Override public void clear(int startIndex, int endIndex) { - throw new UnsupportedOperationException("Not implemented yet"); + throw new UnsupportedOperationException(); } @Override public void clear(int index) { - throw new UnsupportedOperationException("Not implemented yet"); + throw new UnsupportedOperationException(); } @Override @@ -86,20 +89,19 @@ public int length() { @Override public int prevSetBit(int index) { - throw new UnsupportedOperationException("Not implemented yet"); + throw new UnsupportedOperationException(); } @Override public int nextSetBit(int index) { - throw new UnsupportedOperationException("Not implemented yet"); + throw new UnsupportedOperationException(); } @Override public long ramBytesUsed() { - throw new UnsupportedOperationException("Not implemented yet"); + return BASE_RAM_BYTES_USED + (bitset == null ? 0 : bitset.ramBytesUsed()); } - // Exposed for testing boolean isInternalBitsetReleased() { return bitset == null; } diff --git a/core/src/test/java/org/elasticsearch/index/translog/CountedBitSetTests.java b/core/src/test/java/org/elasticsearch/index/translog/CountedBitSetTests.java index 5174d1755bed3..0b138c59eba00 100644 --- a/core/src/test/java/org/elasticsearch/index/translog/CountedBitSetTests.java +++ b/core/src/test/java/org/elasticsearch/index/translog/CountedBitSetTests.java @@ -27,6 +27,7 @@ import java.util.stream.IntStream; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.lessThan; public class CountedBitSetTests extends ESTestCase { @@ -53,6 +54,7 @@ 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 withBitSetRamUsage = countedBitSet.ramBytesUsed(); for (int i = 1; i < numBits; i++) { final int value = values.get(i); @@ -65,6 +67,7 @@ public void testReleaseInternalBitSet() { assertThat(countedBitSet.isInternalBitsetReleased(), equalTo(false)); assertThat(countedBitSet.length(), equalTo(numBits)); assertThat(countedBitSet.cardinality(), equalTo(i)); + assertThat(countedBitSet.ramBytesUsed(), equalTo(withBitSetRamUsage)); } // The missing piece to fill all bits. @@ -79,6 +82,7 @@ public void testReleaseInternalBitSet() { assertThat(countedBitSet.isInternalBitsetReleased(), equalTo(true)); assertThat(countedBitSet.length(), equalTo(numBits)); assertThat(countedBitSet.cardinality(), equalTo(numBits)); + assertThat(countedBitSet.ramBytesUsed(), lessThan(withBitSetRamUsage)); } // Tests with released internal bitset. From 93374d6dc0985162db5f399f0b3d98c88542e282 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Sun, 3 Dec 2017 15:56:22 -0500 Subject: [PATCH 2/3] withBitSetRamUsage -> ramBytesUsedWithBitSet --- .../elasticsearch/index/translog/CountedBitSetTests.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/test/java/org/elasticsearch/index/translog/CountedBitSetTests.java b/core/src/test/java/org/elasticsearch/index/translog/CountedBitSetTests.java index 0b138c59eba00..3293038565dae 100644 --- a/core/src/test/java/org/elasticsearch/index/translog/CountedBitSetTests.java +++ b/core/src/test/java/org/elasticsearch/index/translog/CountedBitSetTests.java @@ -54,7 +54,7 @@ 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 withBitSetRamUsage = countedBitSet.ramBytesUsed(); + final long ramBytesUsedWithBitSet = countedBitSet.ramBytesUsed(); for (int i = 1; i < numBits; i++) { final int value = values.get(i); @@ -67,7 +67,7 @@ public void testReleaseInternalBitSet() { assertThat(countedBitSet.isInternalBitsetReleased(), equalTo(false)); assertThat(countedBitSet.length(), equalTo(numBits)); assertThat(countedBitSet.cardinality(), equalTo(i)); - assertThat(countedBitSet.ramBytesUsed(), equalTo(withBitSetRamUsage)); + assertThat(countedBitSet.ramBytesUsed(), equalTo(ramBytesUsedWithBitSet)); } // The missing piece to fill all bits. @@ -82,7 +82,7 @@ public void testReleaseInternalBitSet() { assertThat(countedBitSet.isInternalBitsetReleased(), equalTo(true)); assertThat(countedBitSet.length(), equalTo(numBits)); assertThat(countedBitSet.cardinality(), equalTo(numBits)); - assertThat(countedBitSet.ramBytesUsed(), lessThan(withBitSetRamUsage)); + assertThat(countedBitSet.ramBytesUsed(), lessThan(ramBytesUsedWithBitSet)); } // Tests with released internal bitset. From 8dbf71dedccbaa0878cb46b3b0782d9af846abb9 Mon Sep 17 00:00:00 2001 From: Nhat Nguyen Date: Sun, 3 Dec 2017 20:58:34 -0500 Subject: [PATCH 3/3] assert with the base line --- .../java/org/elasticsearch/index/translog/CountedBitSet.java | 2 +- .../org/elasticsearch/index/translog/CountedBitSetTests.java | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/translog/CountedBitSet.java b/core/src/main/java/org/elasticsearch/index/translog/CountedBitSet.java index a2ae3970a39a9..ca1ae279a999d 100644 --- a/core/src/main/java/org/elasticsearch/index/translog/CountedBitSet.java +++ b/core/src/main/java/org/elasticsearch/index/translog/CountedBitSet.java @@ -29,7 +29,7 @@ * from translog as these numbers are likely to form contiguous ranges (eg. filling all bits). */ final class CountedBitSet extends BitSet { - private static final long BASE_RAM_BYTES_USED = RamUsageEstimator.shallowSizeOfInstance(CountedBitSet.class); + static final long BASE_RAM_BYTES_USED = RamUsageEstimator.shallowSizeOfInstance(CountedBitSet.class); private short onBits; // Number of bits are set. private FixedBitSet bitset; diff --git a/core/src/test/java/org/elasticsearch/index/translog/CountedBitSetTests.java b/core/src/test/java/org/elasticsearch/index/translog/CountedBitSetTests.java index 3293038565dae..b68607f02d6bf 100644 --- a/core/src/test/java/org/elasticsearch/index/translog/CountedBitSetTests.java +++ b/core/src/test/java/org/elasticsearch/index/translog/CountedBitSetTests.java @@ -26,6 +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; @@ -82,7 +83,7 @@ public void testReleaseInternalBitSet() { assertThat(countedBitSet.isInternalBitsetReleased(), equalTo(true)); assertThat(countedBitSet.length(), equalTo(numBits)); assertThat(countedBitSet.cardinality(), equalTo(numBits)); - assertThat(countedBitSet.ramBytesUsed(), lessThan(ramBytesUsedWithBitSet)); + assertThat(countedBitSet.ramBytesUsed(), allOf(equalTo(CountedBitSet.BASE_RAM_BYTES_USED), lessThan(ramBytesUsedWithBitSet))); } // Tests with released internal bitset.