From 5efa1364f4e99fd40a4fa9189b84b44484b57c91 Mon Sep 17 00:00:00 2001 From: "Colin P. Mccabe" Date: Tue, 16 Mar 2021 14:01:30 -0700 Subject: [PATCH 1/4] MINOR: Fix BaseHashTable sizing The array backing BaseHashTable is intended to be sized as a power of two. Due to a bug, the initial array size was calculated incorrectly in some cases. Also make the maximum array size the largest possible 31-bit power of two. Previously it was a smaller size but this was due to a typo. --- .../apache/kafka/timeline/BaseHashTable.java | 34 +++++++++++++++---- .../kafka/timeline/BaseHashTableTest.java | 15 ++++++++ 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java b/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java index 7183528173cc4..3f71a48debe69 100644 --- a/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java +++ b/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java @@ -40,14 +40,16 @@ class BaseHashTable { private final static double MAX_LOAD_FACTOR = 0.75f; /** - * The natural log of 2 + * The minimum number of slots we can have in the hash table. */ - private final static double LN_2 = Math.log(2); + final static int MIN_CAPACITY = 2; /** * The maximum number of slots we can have in the hash table. + * + * This is set to the maximum 31-bit power of two. */ - private final static int MAX_CAPACITY = 0x4000000; + final static int MAX_CAPACITY = 0x4000_0000; private Object[] elements; private int size = 0; @@ -56,12 +58,30 @@ class BaseHashTable { this.elements = new Object[expectedSizeToCapacity(expectedSize)]; } + /** + * Calculate the capacity we should provision, given the expected size. + * + * Our capacity must always be a power of 2, and never less than 2. + */ static int expectedSizeToCapacity(int expectedSize) { - if (expectedSize <= 1) { - return 2; + if (expectedSize >= MAX_CAPACITY / 2) { + return MAX_CAPACITY; + } + return Math.max(MIN_CAPACITY, roundUpToPowerOfTwo(expectedSize * 2)); + } + + private static int roundUpToPowerOfTwo(int i) { + if (i < 0) { + return 0; } - double sizeToFit = expectedSize / MAX_LOAD_FACTOR; - return (int) Math.min(MAX_CAPACITY, Math.ceil(Math.log(sizeToFit) / LN_2)); + i = i - 1; + i |= i >> 1; + i |= i >> 2; + i |= i >> 4; + i |= i >> 8; + i |= i >> 16; + i = i + 1; + return i < 0 ? MAX_CAPACITY : i; } final int baseSize() { diff --git a/metadata/src/test/java/org/apache/kafka/timeline/BaseHashTableTest.java b/metadata/src/test/java/org/apache/kafka/timeline/BaseHashTableTest.java index 11f774cab7bdf..f35204eff8500 100644 --- a/metadata/src/test/java/org/apache/kafka/timeline/BaseHashTableTest.java +++ b/metadata/src/test/java/org/apache/kafka/timeline/BaseHashTableTest.java @@ -121,4 +121,19 @@ public void testExpansion() { assertEquals(Integer.valueOf(i), table.baseRemove(Integer.valueOf(i))); } } + + @Test + public void testExpectedSizeToCapacity() { + assertEquals(2, BaseHashTable.expectedSizeToCapacity(-123)); + assertEquals(2, BaseHashTable.expectedSizeToCapacity(0)); + assertEquals(2, BaseHashTable.expectedSizeToCapacity(1)); + assertEquals(4, BaseHashTable.expectedSizeToCapacity(2)); + assertEquals(8, BaseHashTable.expectedSizeToCapacity(3)); + assertEquals(8, BaseHashTable.expectedSizeToCapacity(4)); + assertEquals(16, BaseHashTable.expectedSizeToCapacity(5)); + assertEquals(0x4000000, BaseHashTable.expectedSizeToCapacity(0x1010400)); + assertEquals(BaseHashTable.MAX_CAPACITY, BaseHashTable.expectedSizeToCapacity(Integer.MAX_VALUE)); + assertEquals(BaseHashTable.MAX_CAPACITY, BaseHashTable.expectedSizeToCapacity(BaseHashTable.MAX_CAPACITY)); + assertEquals(BaseHashTable.MAX_CAPACITY, BaseHashTable.expectedSizeToCapacity(BaseHashTable.MAX_CAPACITY + 1)); + } } From 1ce473bc5625717fa8e5751603bd44bd61e84d70 Mon Sep 17 00:00:00 2001 From: "Colin P. Mccabe" Date: Wed, 17 Mar 2021 11:12:21 -0700 Subject: [PATCH 2/4] Add somewhat nicer implementation, more test cases --- .../apache/kafka/timeline/BaseHashTable.java | 24 +++++++++---------- .../kafka/timeline/BaseHashTableTest.java | 6 ++++- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java b/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java index 3f71a48debe69..bec6eeac3d2a8 100644 --- a/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java +++ b/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java @@ -44,12 +44,15 @@ class BaseHashTable { */ final static int MIN_CAPACITY = 2; + /** + * The maximum 31-bit power of two. + */ + private final static int MAX_SIGNED_POWER_OF_TWO = 0x4000_0000; + /** * The maximum number of slots we can have in the hash table. - * - * This is set to the maximum 31-bit power of two. */ - final static int MAX_CAPACITY = 0x4000_0000; + final static int MAX_CAPACITY = MAX_SIGNED_POWER_OF_TWO; private Object[] elements; private int size = 0; @@ -71,17 +74,14 @@ static int expectedSizeToCapacity(int expectedSize) { } private static int roundUpToPowerOfTwo(int i) { - if (i < 0) { + if (i <= 0) { return 0; + } else if (i > MAX_SIGNED_POWER_OF_TWO) { + throw new ArithmeticException("There is no 31-bit power of two higher than " + + "or equal to " + i); + } else { + return 1 << -Integer.numberOfLeadingZeros(i - 1); } - i = i - 1; - i |= i >> 1; - i |= i >> 2; - i |= i >> 4; - i |= i >> 8; - i |= i >> 16; - i = i + 1; - return i < 0 ? MAX_CAPACITY : i; } final int baseSize() { diff --git a/metadata/src/test/java/org/apache/kafka/timeline/BaseHashTableTest.java b/metadata/src/test/java/org/apache/kafka/timeline/BaseHashTableTest.java index f35204eff8500..641192da90589 100644 --- a/metadata/src/test/java/org/apache/kafka/timeline/BaseHashTableTest.java +++ b/metadata/src/test/java/org/apache/kafka/timeline/BaseHashTableTest.java @@ -124,6 +124,7 @@ public void testExpansion() { @Test public void testExpectedSizeToCapacity() { + assertEquals(2, BaseHashTable.expectedSizeToCapacity(Integer.MIN_VALUE)); assertEquals(2, BaseHashTable.expectedSizeToCapacity(-123)); assertEquals(2, BaseHashTable.expectedSizeToCapacity(0)); assertEquals(2, BaseHashTable.expectedSizeToCapacity(1)); @@ -132,8 +133,11 @@ public void testExpectedSizeToCapacity() { assertEquals(8, BaseHashTable.expectedSizeToCapacity(4)); assertEquals(16, BaseHashTable.expectedSizeToCapacity(5)); assertEquals(0x4000000, BaseHashTable.expectedSizeToCapacity(0x1010400)); - assertEquals(BaseHashTable.MAX_CAPACITY, BaseHashTable.expectedSizeToCapacity(Integer.MAX_VALUE)); + assertEquals(0x4000000, BaseHashTable.expectedSizeToCapacity(0x2000000)); + assertEquals(0x8000000, BaseHashTable.expectedSizeToCapacity(0x2000001)); assertEquals(BaseHashTable.MAX_CAPACITY, BaseHashTable.expectedSizeToCapacity(BaseHashTable.MAX_CAPACITY)); assertEquals(BaseHashTable.MAX_CAPACITY, BaseHashTable.expectedSizeToCapacity(BaseHashTable.MAX_CAPACITY + 1)); + assertEquals(BaseHashTable.MAX_CAPACITY, BaseHashTable.expectedSizeToCapacity(Integer.MAX_VALUE - 1)); + assertEquals(BaseHashTable.MAX_CAPACITY, BaseHashTable.expectedSizeToCapacity(Integer.MAX_VALUE)); } } From 7cd7e3c26f1cd1aea722d084ffc2f4e9fb3583a9 Mon Sep 17 00:00:00 2001 From: "Colin P. Mccabe" Date: Wed, 17 Mar 2021 14:23:40 -0700 Subject: [PATCH 3/4] Use load factor in initial sizing --- .../apache/kafka/timeline/BaseHashTable.java | 27 +++++++++---------- .../kafka/timeline/BaseHashTableTest.java | 9 ++++--- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java b/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java index bec6eeac3d2a8..4532b675eccca 100644 --- a/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java +++ b/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java @@ -44,15 +44,10 @@ class BaseHashTable { */ final static int MIN_CAPACITY = 2; - /** - * The maximum 31-bit power of two. - */ - private final static int MAX_SIGNED_POWER_OF_TWO = 0x4000_0000; - /** * The maximum number of slots we can have in the hash table. */ - final static int MAX_CAPACITY = MAX_SIGNED_POWER_OF_TWO; + final static int MAX_CAPACITY = 1 << 30; private Object[] elements; private int size = 0; @@ -64,23 +59,25 @@ class BaseHashTable { /** * Calculate the capacity we should provision, given the expected size. * - * Our capacity must always be a power of 2, and never less than 2. + * Our capacity must always be a power of 2, and never less than 2 or more + * than MAX_CAPACITY. We use 64-bit numbers here to avoid overflow + * concerns. */ + @SuppressWarnings("unchecked") static int expectedSizeToCapacity(int expectedSize) { - if (expectedSize >= MAX_CAPACITY / 2) { - return MAX_CAPACITY; - } - return Math.max(MIN_CAPACITY, roundUpToPowerOfTwo(expectedSize * 2)); + long minCapacity = (long) Math.ceil((float) expectedSize / MAX_LOAD_FACTOR); + return Math.max(MIN_CAPACITY, + (int) Math.min(MAX_CAPACITY, roundUpToPowerOfTwo(minCapacity))); } - private static int roundUpToPowerOfTwo(int i) { + private static long roundUpToPowerOfTwo(long i) { if (i <= 0) { return 0; - } else if (i > MAX_SIGNED_POWER_OF_TWO) { - throw new ArithmeticException("There is no 31-bit power of two higher than " + + } else if (i > (1L << 62)) { + throw new ArithmeticException("There are no 63-bit powers of 2 higher than " + "or equal to " + i); } else { - return 1 << -Integer.numberOfLeadingZeros(i - 1); + return 1L << -Long.numberOfLeadingZeros(i - 1); } } diff --git a/metadata/src/test/java/org/apache/kafka/timeline/BaseHashTableTest.java b/metadata/src/test/java/org/apache/kafka/timeline/BaseHashTableTest.java index 641192da90589..a73357c5234a9 100644 --- a/metadata/src/test/java/org/apache/kafka/timeline/BaseHashTableTest.java +++ b/metadata/src/test/java/org/apache/kafka/timeline/BaseHashTableTest.java @@ -129,12 +129,13 @@ public void testExpectedSizeToCapacity() { assertEquals(2, BaseHashTable.expectedSizeToCapacity(0)); assertEquals(2, BaseHashTable.expectedSizeToCapacity(1)); assertEquals(4, BaseHashTable.expectedSizeToCapacity(2)); - assertEquals(8, BaseHashTable.expectedSizeToCapacity(3)); + assertEquals(4, BaseHashTable.expectedSizeToCapacity(3)); assertEquals(8, BaseHashTable.expectedSizeToCapacity(4)); - assertEquals(16, BaseHashTable.expectedSizeToCapacity(5)); - assertEquals(0x4000000, BaseHashTable.expectedSizeToCapacity(0x1010400)); + assertEquals(16, BaseHashTable.expectedSizeToCapacity(12)); + assertEquals(32, BaseHashTable.expectedSizeToCapacity(13)); + assertEquals(0x2000000, BaseHashTable.expectedSizeToCapacity(0x1010400)); assertEquals(0x4000000, BaseHashTable.expectedSizeToCapacity(0x2000000)); - assertEquals(0x8000000, BaseHashTable.expectedSizeToCapacity(0x2000001)); + assertEquals(0x4000000, BaseHashTable.expectedSizeToCapacity(0x2000001)); assertEquals(BaseHashTable.MAX_CAPACITY, BaseHashTable.expectedSizeToCapacity(BaseHashTable.MAX_CAPACITY)); assertEquals(BaseHashTable.MAX_CAPACITY, BaseHashTable.expectedSizeToCapacity(BaseHashTable.MAX_CAPACITY + 1)); assertEquals(BaseHashTable.MAX_CAPACITY, BaseHashTable.expectedSizeToCapacity(Integer.MAX_VALUE - 1)); From 8d9e52393db9641a64d7095c69b9987a75188413 Mon Sep 17 00:00:00 2001 From: "Colin P. Mccabe" Date: Thu, 18 Mar 2021 09:55:18 -0700 Subject: [PATCH 4/4] Unchecked not needed in expectedSizeToCapacity --- .../src/main/java/org/apache/kafka/timeline/BaseHashTable.java | 1 - 1 file changed, 1 deletion(-) diff --git a/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java b/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java index 4532b675eccca..0531546651903 100644 --- a/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java +++ b/metadata/src/main/java/org/apache/kafka/timeline/BaseHashTable.java @@ -63,7 +63,6 @@ class BaseHashTable { * than MAX_CAPACITY. We use 64-bit numbers here to avoid overflow * concerns. */ - @SuppressWarnings("unchecked") static int expectedSizeToCapacity(int expectedSize) { long minCapacity = (long) Math.ceil((float) expectedSize / MAX_LOAD_FACTOR); return Math.max(MIN_CAPACITY,