From 16e0a63077bbfe889b7150543c5f176e0f637682 Mon Sep 17 00:00:00 2001 From: Mark Hansen Date: Tue, 21 May 2024 22:25:24 -0700 Subject: [PATCH] Remove field SmallSortedMap.maxArraySize It's always set to 16 in production code. Except for when it's set to 0 for the immutable empty SmallSortedMap, but the number doesn't change anything for that immutable empty map (it's always empty, it will never hit the max). This should save 4 bytes of memory per SmallSortedMap. PiperOrigin-RevId: 636035193 --- .../java/com/google/protobuf/FieldSet.java | 10 +- .../com/google/protobuf/SmallSortedMap.java | 33 ++-- .../google/protobuf/SmallSortedMapTest.java | 144 +++++++++--------- 3 files changed, 88 insertions(+), 99 deletions(-) diff --git a/java/core/src/main/java/com/google/protobuf/FieldSet.java b/java/core/src/main/java/com/google/protobuf/FieldSet.java index 4c12486bd553..85b1ad0c64fa 100644 --- a/java/core/src/main/java/com/google/protobuf/FieldSet.java +++ b/java/core/src/main/java/com/google/protobuf/FieldSet.java @@ -49,21 +49,19 @@ public interface FieldDescriptorLite> extends C MessageLite.Builder internalMergeFrom(MessageLite.Builder to, MessageLite from); } - private static final int DEFAULT_FIELD_MAP_ARRAY_SIZE = 16; - private final SmallSortedMap fields; private boolean isImmutable; private boolean hasLazyField; /** Construct a new FieldSet. */ private FieldSet() { - this.fields = SmallSortedMap.newFieldMap(DEFAULT_FIELD_MAP_ARRAY_SIZE); + this.fields = SmallSortedMap.newFieldMap(); } /** Construct an empty FieldSet. This is only used to initialize DEFAULT_INSTANCE. */ @SuppressWarnings("unused") private FieldSet(final boolean dummy) { - this(SmallSortedMap.newFieldMap(0)); + this(SmallSortedMap.newFieldMap()); makeImmutable(); } @@ -185,7 +183,7 @@ public Map getAllFields() { private static > SmallSortedMap cloneAllFieldsMap( SmallSortedMap fields, boolean copyList, boolean resolveLazyFields) { - SmallSortedMap result = SmallSortedMap.newFieldMap(DEFAULT_FIELD_MAP_ARRAY_SIZE); + SmallSortedMap result = SmallSortedMap.newFieldMap(); for (int i = 0; i < fields.getNumArrayEntries(); i++) { cloneFieldEntry(result, fields.getArrayEntryAt(i), copyList, resolveLazyFields); } @@ -931,7 +929,7 @@ static final class Builder> { private boolean hasNestedBuilders; private Builder() { - this(SmallSortedMap.newFieldMap(DEFAULT_FIELD_MAP_ARRAY_SIZE)); + this(SmallSortedMap.newFieldMap()); } private Builder(SmallSortedMap fields) { diff --git a/java/core/src/main/java/com/google/protobuf/SmallSortedMap.java b/java/core/src/main/java/com/google/protobuf/SmallSortedMap.java index 495417dd0db3..c7de130c49e3 100644 --- a/java/core/src/main/java/com/google/protobuf/SmallSortedMap.java +++ b/java/core/src/main/java/com/google/protobuf/SmallSortedMap.java @@ -58,6 +58,8 @@ // a subclass to aid testability of the core logic. class SmallSortedMap, V> extends AbstractMap { + static final int DEFAULT_FIELD_MAP_ARRAY_SIZE = 16; + /** * Creates a new instance for mapping FieldDescriptors to their values. The {@link * #makeImmutable()} implementation will convert the List values of any repeated fields to @@ -67,8 +69,8 @@ class SmallSortedMap, V> extends AbstractMap { * mappings. */ static > - SmallSortedMap newFieldMap(int arraySize) { - return new SmallSortedMap(arraySize) { + SmallSortedMap newFieldMap() { + return new SmallSortedMap() { @Override @SuppressWarnings("unchecked") public void makeImmutable() { @@ -76,13 +78,13 @@ public void makeImmutable() { for (int i = 0; i < getNumArrayEntries(); i++) { final Map.Entry entry = getArrayEntryAt(i); if (entry.getKey().isRepeated()) { - final List value = (List) entry.getValue(); + final List value = (List) entry.getValue(); entry.setValue(Collections.unmodifiableList(value)); } } for (Map.Entry entry : getOverflowEntries()) { if (entry.getKey().isRepeated()) { - final List value = (List) entry.getValue(); + final List value = (List) entry.getValue(); entry.setValue(Collections.unmodifiableList(value)); } } @@ -92,17 +94,11 @@ public void makeImmutable() { }; } - /** - * Creates a new instance for testing. - * - * @param arraySize The size of the entry array containing the lexicographically smallest - * mappings. - */ - static , V> SmallSortedMap newInstanceForTest(int arraySize) { - return new SmallSortedMap(arraySize); + /** Creates a new instance for testing. */ + static , V> SmallSortedMap newInstanceForTest() { + return new SmallSortedMap<>(); } - private final int maxArraySize; // The "entry array" is actually a List because generic arrays are not // allowed. ArrayList also nicely handles the entry shifting on inserts and // removes. @@ -119,8 +115,7 @@ static , V> SmallSortedMap newInstanceForTest(int * @code arraySize Size of the array in which the lexicographically smallest mappings are stored. * (i.e. the {@code k} referred to in the class documentation). */ - private SmallSortedMap(int arraySize) { - this.maxArraySize = arraySize; + private SmallSortedMap() { this.entryList = Collections.emptyList(); this.overflowEntries = Collections.emptyMap(); this.overflowEntriesDescending = Collections.emptyMap(); @@ -215,14 +210,14 @@ public V put(K key, V value) { } ensureEntryArrayMutable(); final int insertionPoint = -(index + 1); - if (insertionPoint >= maxArraySize) { + if (insertionPoint >= DEFAULT_FIELD_MAP_ARRAY_SIZE) { // Put directly in overflow. return getOverflowEntriesMutable().put(key, value); } // Insert new Entry in array. - if (entryList.size() == maxArraySize) { + if (entryList.size() == DEFAULT_FIELD_MAP_ARRAY_SIZE) { // Shift the last array entry into overflow. - final Entry lastEntryInArray = entryList.remove(maxArraySize - 1); + final Entry lastEntryInArray = entryList.remove(DEFAULT_FIELD_MAP_ARRAY_SIZE - 1); getOverflowEntriesMutable().put(lastEntryInArray.getKey(), lastEntryInArray.getValue()); } entryList.add(insertionPoint, new Entry(key, value)); @@ -357,7 +352,7 @@ private SortedMap getOverflowEntriesMutable() { private void ensureEntryArrayMutable() { checkMutable(); if (entryList.isEmpty() && !(entryList instanceof ArrayList)) { - entryList = new ArrayList(maxArraySize); + entryList = new ArrayList<>(DEFAULT_FIELD_MAP_ARRAY_SIZE); } } diff --git a/java/core/src/test/java/com/google/protobuf/SmallSortedMapTest.java b/java/core/src/test/java/com/google/protobuf/SmallSortedMapTest.java index dbac08abaf33..a2f04b987f9e 100644 --- a/java/core/src/test/java/com/google/protobuf/SmallSortedMapTest.java +++ b/java/core/src/test/java/com/google/protobuf/SmallSortedMapTest.java @@ -9,6 +9,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; +import static com.google.protobuf.SmallSortedMap.DEFAULT_FIELD_MAP_ARRAY_SIZE; import static java.lang.Math.min; import java.util.AbstractMap; @@ -29,42 +30,33 @@ public class SmallSortedMapTest { @Test public void testPutAndGetArrayEntriesOnly() { - runPutAndGetTest(3); + runPutAndGetTest(DEFAULT_FIELD_MAP_ARRAY_SIZE); } @Test public void testPutAndGetOverflowEntries() { - runPutAndGetTest(6); + runPutAndGetTest(DEFAULT_FIELD_MAP_ARRAY_SIZE * 2); } private void runPutAndGetTest(int numElements) { - // Test with even and odd arraySize - SmallSortedMap map1 = SmallSortedMap.newInstanceForTest(3); - SmallSortedMap map2 = SmallSortedMap.newInstanceForTest(4); - SmallSortedMap map3 = SmallSortedMap.newInstanceForTest(3); - SmallSortedMap map4 = SmallSortedMap.newInstanceForTest(4); + SmallSortedMap map1 = SmallSortedMap.newInstanceForTest(); + SmallSortedMap map3 = SmallSortedMap.newInstanceForTest(); // Test with puts in ascending order. for (int i = 0; i < numElements; i++) { assertThat(map1.put(i, i + 1)).isNull(); - assertThat(map2.put(i, i + 1)).isNull(); } // Test with puts in descending order. for (int i = numElements - 1; i >= 0; i--) { assertThat(map3.put(i, i + 1)).isNull(); - assertThat(map4.put(i, i + 1)).isNull(); } - assertThat(map1.getNumArrayEntries()).isEqualTo(min(3, numElements)); - assertThat(map2.getNumArrayEntries()).isEqualTo(min(4, numElements)); - assertThat(map3.getNumArrayEntries()).isEqualTo(min(3, numElements)); - assertThat(map4.getNumArrayEntries()).isEqualTo(min(4, numElements)); + assertThat(map1.getNumArrayEntries()).isEqualTo(min(16, numElements)); + assertThat(map3.getNumArrayEntries()).isEqualTo(min(16, numElements)); List> allMaps = new ArrayList<>(); allMaps.add(map1); - allMaps.add(map2); allMaps.add(map3); - allMaps.add(map4); for (SmallSortedMap map : allMaps) { assertThat(map).hasSize(numElements); @@ -73,69 +65,72 @@ private void runPutAndGetTest(int numElements) { } } - assertThat(map1).isEqualTo(map2); - assertThat(map2).isEqualTo(map3); - assertThat(map3).isEqualTo(map4); + assertThat(map1).isEqualTo(map3); } @Test public void testReplacingPut() { - SmallSortedMap map = SmallSortedMap.newInstanceForTest(3); - for (int i = 0; i < 6; i++) { + SmallSortedMap map = SmallSortedMap.newInstanceForTest(); + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(map.put(i, i + 1)).isNull(); assertThat(map.remove(i + 1)).isNull(); } - for (int i = 0; i < 6; i++) { + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(map.put(i, i + 2)).isEqualTo(Integer.valueOf(i + 1)); } } @Test public void testRemove() { - SmallSortedMap map = SmallSortedMap.newInstanceForTest(3); - for (int i = 0; i < 6; i++) { + SmallSortedMap map = SmallSortedMap.newInstanceForTest(); + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE + 3; i++) { assertThat(map.put(i, i + 1)).isNull(); assertThat(map.remove(i + 1)).isNull(); } - assertThat(map.getNumArrayEntries()).isEqualTo(3); + assertThat(map.getNumArrayEntries()).isEqualTo(16); assertThat(map.getNumOverflowEntries()).isEqualTo(3); - assertThat(map).hasSize(6); - assertThat(map.keySet()).isEqualTo(makeSortedKeySet(0, 1, 2, 3, 4, 5)); + assertThat(map).hasSize(19); + assertThat(map.keySet()) + .isEqualTo( + makeSortedKeySet(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18)); assertThat(map.remove(1)).isEqualTo(Integer.valueOf(2)); - assertThat(map.getNumArrayEntries()).isEqualTo(3); + assertThat(map.getNumArrayEntries()).isEqualTo(16); assertThat(map.getNumOverflowEntries()).isEqualTo(2); - assertThat(map).hasSize(5); - assertThat(map.keySet()).isEqualTo(makeSortedKeySet(0, 2, 3, 4, 5)); + assertThat(map).hasSize(18); + assertThat(map.keySet()) + .isEqualTo(makeSortedKeySet(0, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18)); assertThat(map.remove(4)).isEqualTo(Integer.valueOf(5)); - assertThat(map.getNumArrayEntries()).isEqualTo(3); + assertThat(map.getNumArrayEntries()).isEqualTo(16); assertThat(map.getNumOverflowEntries()).isEqualTo(1); - assertThat(map).hasSize(4); - assertThat(map.keySet()).isEqualTo(makeSortedKeySet(0, 2, 3, 5)); + assertThat(map).hasSize(17); + assertThat(map.keySet()) + .isEqualTo(makeSortedKeySet(0, 2, 3, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18)); assertThat(map.remove(3)).isEqualTo(Integer.valueOf(4)); - assertThat(map.getNumArrayEntries()).isEqualTo(3); + assertThat(map.getNumArrayEntries()).isEqualTo(16); assertThat(map.getNumOverflowEntries()).isEqualTo(0); - assertThat(map).hasSize(3); - assertThat(map.keySet()).isEqualTo(makeSortedKeySet(0, 2, 5)); + assertThat(map).hasSize(16); + assertThat(map.keySet()) + .isEqualTo(makeSortedKeySet(0, 2, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18)); assertThat(map.remove(3)).isNull(); - assertThat(map.getNumArrayEntries()).isEqualTo(3); + assertThat(map.getNumArrayEntries()).isEqualTo(16); assertThat(map.getNumOverflowEntries()).isEqualTo(0); - assertThat(map).hasSize(3); + assertThat(map).hasSize(16); assertThat(map.remove(0)).isEqualTo(Integer.valueOf(1)); - assertThat(map.getNumArrayEntries()).isEqualTo(2); + assertThat(map.getNumArrayEntries()).isEqualTo(15); assertThat(map.getNumOverflowEntries()).isEqualTo(0); - assertThat(map).hasSize(2); + assertThat(map).hasSize(15); } @Test public void testClear() { - SmallSortedMap map = SmallSortedMap.newInstanceForTest(3); - for (int i = 0; i < 6; i++) { + SmallSortedMap map = SmallSortedMap.newInstanceForTest(); + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(map.put(i, i + 1)).isNull(); } map.clear(); @@ -146,18 +141,19 @@ public void testClear() { @Test public void testGetArrayEntryAndOverflowEntries() { - SmallSortedMap map = SmallSortedMap.newInstanceForTest(3); - for (int i = 0; i < 6; i++) { + SmallSortedMap map = SmallSortedMap.newInstanceForTest(); + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(map.put(i, i + 1)).isNull(); } - assertThat(map.getNumArrayEntries()).isEqualTo(3); - for (int i = 0; i < 3; i++) { + assertThat(map.getNumArrayEntries()).isEqualTo(DEFAULT_FIELD_MAP_ARRAY_SIZE); + for (int i = 0; i < map.getNumArrayEntries(); i++) { Map.Entry entry = map.getArrayEntryAt(i); assertThat(entry.getKey()).isEqualTo(Integer.valueOf(i)); assertThat(entry.getValue()).isEqualTo(Integer.valueOf(i + 1)); } Iterator> it = map.getOverflowEntries().iterator(); - for (int i = 3; i < 6; i++) { + assertThat(map.getNumOverflowEntries()).isEqualTo(DEFAULT_FIELD_MAP_ARRAY_SIZE); + for (int i = DEFAULT_FIELD_MAP_ARRAY_SIZE; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(it.hasNext()).isTrue(); Map.Entry entry = it.next(); assertThat(entry.getKey()).isEqualTo(Integer.valueOf(i)); @@ -168,12 +164,12 @@ public void testGetArrayEntryAndOverflowEntries() { @Test public void testEntrySetContains() { - SmallSortedMap map = SmallSortedMap.newInstanceForTest(3); - for (int i = 0; i < 6; i++) { + SmallSortedMap map = SmallSortedMap.newInstanceForTest(); + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(map.put(i, i + 1)).isNull(); } Set> entrySet = map.entrySet(); - for (int i = 0; i < 6; i++) { + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(entrySet).contains(new AbstractMap.SimpleEntry(i, i + 1)); assertThat(entrySet).doesNotContain(new AbstractMap.SimpleEntry(i, i)); } @@ -181,29 +177,29 @@ public void testEntrySetContains() { @Test public void testEntrySetAdd() { - SmallSortedMap map = SmallSortedMap.newInstanceForTest(3); + SmallSortedMap map = SmallSortedMap.newInstanceForTest(); Set> entrySet = map.entrySet(); - for (int i = 0; i < 6; i++) { + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { Map.Entry entry = new AbstractMap.SimpleEntry<>(i, i + 1); assertThat(entrySet.add(entry)).isTrue(); assertThat(entrySet.add(entry)).isFalse(); } - for (int i = 0; i < 6; i++) { + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(map).containsEntry(i, Integer.valueOf(i + 1)); } - assertThat(map.getNumArrayEntries()).isEqualTo(3); - assertThat(map.getNumOverflowEntries()).isEqualTo(3); - assertThat(map).hasSize(6); + assertThat(map.getNumArrayEntries()).isEqualTo(16); + assertThat(map.getNumOverflowEntries()).isEqualTo(16); + assertThat(map).hasSize(32); } @Test public void testEntrySetRemove() { - SmallSortedMap map = SmallSortedMap.newInstanceForTest(3); + SmallSortedMap map = SmallSortedMap.newInstanceForTest(); Set> entrySet = map.entrySet(); - for (int i = 0; i < 6; i++) { + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(map.put(i, i + 1)).isNull(); } - for (int i = 0; i < 6; i++) { + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { Map.Entry entry = new AbstractMap.SimpleEntry<>(i, i + 1); assertThat(entrySet.remove(entry)).isTrue(); assertThat(entrySet.remove(entry)).isFalse(); @@ -216,8 +212,8 @@ public void testEntrySetRemove() { @Test public void testEntrySetClear() { - SmallSortedMap map = SmallSortedMap.newInstanceForTest(3); - for (int i = 0; i < 6; i++) { + SmallSortedMap map = SmallSortedMap.newInstanceForTest(); + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(map.put(i, i + 1)).isNull(); } map.clear(); @@ -229,12 +225,12 @@ public void testEntrySetClear() { @Test public void testEntrySetIteratorNext() { - SmallSortedMap map = SmallSortedMap.newInstanceForTest(3); - for (int i = 0; i < 6; i++) { + SmallSortedMap map = SmallSortedMap.newInstanceForTest(); + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(map.put(i, i + 1)).isNull(); } Iterator> it = map.entrySet().iterator(); - for (int i = 0; i < 6; i++) { + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(it.hasNext()).isTrue(); Map.Entry entry = it.next(); assertThat(entry.getKey()).isEqualTo(Integer.valueOf(i)); @@ -245,45 +241,45 @@ public void testEntrySetIteratorNext() { @Test public void testEntrySetIteratorRemove() { - SmallSortedMap map = SmallSortedMap.newInstanceForTest(3); - for (int i = 0; i < 6; i++) { + SmallSortedMap map = SmallSortedMap.newInstanceForTest(); + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(map.put(i, i + 1)).isNull(); } Iterator> it = map.entrySet().iterator(); - for (int i = 0; i < 6; i++) { + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(map).containsKey(i); it.next(); it.remove(); assertThat(map).doesNotContainKey(i); - assertThat(map).hasSize(6 - i - 1); + assertThat(map).hasSize(32 - i - 1); } } @Test public void testMapEntryModification() { - SmallSortedMap map = SmallSortedMap.newInstanceForTest(3); - for (int i = 0; i < 6; i++) { + SmallSortedMap map = SmallSortedMap.newInstanceForTest(); + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(map.put(i, i + 1)).isNull(); } Iterator> it = map.entrySet().iterator(); - for (int i = 0; i < 6; i++) { + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { Map.Entry entry = it.next(); entry.setValue(i + 23); } - for (int i = 0; i < 6; i++) { + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(map).containsEntry(i, Integer.valueOf(i + 23)); } } @Test public void testMakeImmutable() { - SmallSortedMap map = SmallSortedMap.newInstanceForTest(3); - for (int i = 0; i < 6; i++) { + SmallSortedMap map = SmallSortedMap.newInstanceForTest(); + for (int i = 0; i < DEFAULT_FIELD_MAP_ARRAY_SIZE * 2; i++) { assertThat(map.put(i, i + 1)).isNull(); } map.makeImmutable(); assertThat(map).containsEntry(0, Integer.valueOf(1)); - assertThat(map).hasSize(6); + assertThat(map).hasSize(DEFAULT_FIELD_MAP_ARRAY_SIZE * 2); try { map.put(23, 23);