From b2023c55a6c163a399e88957e56ca3cb07d412ed Mon Sep 17 00:00:00 2001 From: jcater Date: Wed, 2 Dec 2020 13:38:14 -0800 Subject: [PATCH] AttributeContainer.Large now handles more than 127 attributes. The previous implementation failed with more than 127 attributes because the call to Arrays.binarySearch treats the byte values as signed, so values of 128 and above wrapped around and became negative, breaking binarySearch's precondition that the array be sorted. Also adds a small test. PiperOrigin-RevId: 345302645 --- .../lib/packages/AttributeContainer.java | 53 +++++++++---------- .../lib/packages/AttributeContainerTest.java | 25 +++++++++ 2 files changed, 51 insertions(+), 27 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/packages/AttributeContainer.java b/src/main/java/com/google/devtools/build/lib/packages/AttributeContainer.java index 8a0fc00585f5f6..8c9f5003bf594c 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AttributeContainer.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AttributeContainer.java @@ -16,7 +16,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.errorprone.annotations.CheckReturnValue; -import java.util.Arrays; import java.util.BitSet; import javax.annotation.Nullable; @@ -154,6 +153,25 @@ private static int nonNullCount(Object[] attrValues) { return numSet; } + /** Returns index into state array for attrIndex, or -1 if not found */ + private static int getStateIndex(byte[] state, int start, int attrIndex, int mask) { + // Binary search, treating values as unsigned bytes. + int lo = start; + int hi = state.length - 1; + while (hi >= lo) { + int mid = (lo + hi) / 2; + int midAttrIndex = state[mid] & mask; + if (midAttrIndex == attrIndex) { + return mid; + } else if (midAttrIndex < attrIndex) { + lo = mid + 1; + } else { + hi = mid - 1; + } + } + return -1; + } + /** * A frozen implementation of AttributeContainer which supports RuleClasses with up to 126 * attributes. @@ -161,7 +179,7 @@ private static int nonNullCount(Object[] attrValues) { @VisibleForTesting static final class Small extends Frozen { - private int maxAttrCount; + private final int maxAttrCount; // Conceptually an AttributeContainer is an unordered set of triples // (attribute, value, explicit). @@ -219,25 +237,6 @@ private Small(Object[] attrValues, BitSet explicitAttrs) { } } - /** Returns index into state array for attrIndex, or -1 if not found */ - private int getStateIndex(int attrIndex) { - // Binary search on the bottom 7 bits. - int lo = 0; - int hi = state.length - 1; - while (hi >= lo) { - int mid = (lo + hi) / 2; - int midAttrIndex = state[mid] & 0x7f; - if (midAttrIndex == attrIndex) { - return mid; - } else if (midAttrIndex < attrIndex) { - lo = mid + 1; - } else { - hi = mid - 1; - } - } - return -1; - } - /** * Returns true iff the value of the specified attribute is explicitly set in the BUILD file. In * addition, this method return false if the rule has no attribute with the given name. @@ -247,7 +246,7 @@ boolean isAttributeValueExplicitlySpecified(int attrIndex) { if (attrIndex < 0) { return false; } - int stateIndex = getStateIndex(attrIndex); + int stateIndex = getStateIndex(state, 0, attrIndex, 0x7f); return stateIndex >= 0 && (state[stateIndex] & 0x80) != 0; } @@ -258,7 +257,7 @@ Object getAttributeValue(int attrIndex) { throw new IndexOutOfBoundsException( "Maximum valid attrIndex is " + (maxAttrCount - 1) + ". Given " + attrIndex); } - int stateIndex = getStateIndex(attrIndex); + int stateIndex = getStateIndex(state, 0, attrIndex, 0x7f); return stateIndex < 0 ? null : values[stateIndex]; } } @@ -308,7 +307,7 @@ private static int prefixSize(int attrCount) { /** Set the specified bit in the byte array. Assumes bitIndex is a valid index. */ private static void setBit(byte[] bits, int bitIndex) { int idx = (bitIndex + 1); - byte explicitByte = bits[idx >> 3]; + int explicitByte = bits[idx >> 3]; byte mask = (byte) (1 << (idx & 0x07)); bits[idx >> 3] = (byte) (explicitByte | mask); } @@ -316,8 +315,8 @@ private static void setBit(byte[] bits, int bitIndex) { /** Get the specified bit in the byte array. Assumes bitIndex is a valid index. */ private static boolean getBit(byte[] bits, int bitIndex) { int idx = (bitIndex + 1); - byte explicitByte = bits[idx >> 3]; - byte mask = (byte) (1 << (idx & 0x07)); + int explicitByte = bits[idx >> 3]; + int mask = (byte) (1 << (idx & 0x07)); return (explicitByte & mask) != 0; } @@ -374,7 +373,7 @@ Object getAttributeValue(int attrIndex) { "Maximum valid attrIndex is " + (maxAttrCount - 1) + ". Given " + attrIndex); } int p = prefixSize(maxAttrCount); - int stateIndex = Arrays.binarySearch(state, p, state.length, (byte) attrIndex); + int stateIndex = getStateIndex(state, p, attrIndex, 0xff); return stateIndex < 0 ? null : values[stateIndex - p]; } } diff --git a/src/test/java/com/google/devtools/build/lib/packages/AttributeContainerTest.java b/src/test/java/com/google/devtools/build/lib/packages/AttributeContainerTest.java index de8be7432a238a..9e1295d6cfb25d 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/AttributeContainerTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/AttributeContainerTest.java @@ -158,4 +158,29 @@ public void testFreezeWorks_smallImplementation() { public void testFreezeWorks_largeImplementation() { checkFreezeWorks((short) 150, AttributeContainer.Large.class); } + + private void testContainerSize(int size) { + AttributeContainer container = new Mutable(size); + for (int i = 0; i < size; i++) { + container.setAttributeValue(i, "value " + i, i % 2 == 0); + } + AttributeContainer frozen = container.freeze(); + // Check values. + for (int i = 0; i < size; i++) { + assertThat(frozen.getAttributeValue(i)).isEqualTo("value " + i); + assertThat(frozen.isAttributeValueExplicitlySpecified(i)).isEqualTo(i % 2 == 0); + } + } + + @Test + public void testSmallContainer() { + // At 127 attributes, we shift from AttributeContainer.Small to AttributeContainer.Large. + testContainerSize(126); + } + + @Test + public void testLargeContainer() { + // AttributeContainer.Large can handle at max 254 attributes. + testContainerSize(254); + } }