From 2f4154d53408b77d3c3e11ccf6936b4be4a81a31 Mon Sep 17 00:00:00 2001 From: cpovirk Date: Fri, 3 May 2024 05:55:42 -0700 Subject: [PATCH] Roll back the most recent `singletonIterator` optimization attempt. (I'll probably just roll all my attempts back.) We got reports of a regression in Bazel. PiperOrigin-RevId: 630366325 --- .../google/common/collect/IteratorsTest.java | 52 +++++-------------- .../com/google/common/collect/Iterators.java | 48 +++++------------ .../google/common/collect/IteratorsTest.java | 52 +++++-------------- .../com/google/common/collect/Iterators.java | 48 +++++------------ 4 files changed, 56 insertions(+), 144 deletions(-) diff --git a/android/guava-tests/test/com/google/common/collect/IteratorsTest.java b/android/guava-tests/test/com/google/common/collect/IteratorsTest.java index 20e542f7c38e..aba34944c1fa 100644 --- a/android/guava-tests/test/com/google/common/collect/IteratorsTest.java +++ b/android/guava-tests/test/com/google/common/collect/IteratorsTest.java @@ -20,7 +20,6 @@ import static com.google.common.collect.Iterators.advance; import static com.google.common.collect.Iterators.get; import static com.google.common.collect.Iterators.getLast; -import static com.google.common.collect.Iterators.singletonIterator; import static com.google.common.collect.Lists.newArrayList; import static com.google.common.collect.testing.IteratorFeature.MODIFIABLE; import static com.google.common.collect.testing.IteratorFeature.UNMODIFIABLE; @@ -801,19 +800,21 @@ protected Iterator newTargetIterator() { } public void testConcatPartiallyAdvancedSecond() { - Iterator itr1 = Iterators.concat(singletonIterator("a"), Iterators.forArray("b", "c")); + Iterator itr1 = + Iterators.concat(Iterators.singletonIterator("a"), Iterators.forArray("b", "c")); assertEquals("a", itr1.next()); assertEquals("b", itr1.next()); - Iterator itr2 = Iterators.concat(singletonIterator("d"), itr1); + Iterator itr2 = Iterators.concat(Iterators.singletonIterator("d"), itr1); assertEquals("d", itr2.next()); assertEquals("c", itr2.next()); } public void testConcatPartiallyAdvancedFirst() { - Iterator itr1 = Iterators.concat(singletonIterator("a"), Iterators.forArray("b", "c")); + Iterator itr1 = + Iterators.concat(Iterators.singletonIterator("a"), Iterators.forArray("b", "c")); assertEquals("a", itr1.next()); assertEquals("b", itr1.next()); - Iterator itr2 = Iterators.concat(itr1, singletonIterator("d")); + Iterator itr2 = Iterators.concat(itr1, Iterators.singletonIterator("d")); assertEquals("c", itr2.next()); assertEquals("d", itr2.next()); } @@ -975,7 +976,7 @@ public void testElementsEqual() { } public void testPartition_badSize() { - Iterator source = singletonIterator(1); + Iterator source = Iterators.singletonIterator(1); try { Iterators.partition(source, 0); fail(); @@ -990,7 +991,7 @@ public void testPartition_empty() { } public void testPartition_singleton1() { - Iterator source = singletonIterator(1); + Iterator source = Iterators.singletonIterator(1); Iterator> partitions = Iterators.partition(source, 1); assertTrue(partitions.hasNext()); assertTrue(partitions.hasNext()); @@ -999,7 +1000,7 @@ public void testPartition_singleton1() { } public void testPartition_singleton2() { - Iterator source = singletonIterator(1); + Iterator source = Iterators.singletonIterator(1); Iterator> partitions = Iterators.partition(source, 2); assertTrue(partitions.hasNext()); assertTrue(partitions.hasNext()); @@ -1046,7 +1047,7 @@ public void testPartitionRandomAccess() { } public void testPaddedPartition_badSize() { - Iterator source = singletonIterator(1); + Iterator source = Iterators.singletonIterator(1); try { Iterators.paddedPartition(source, 0); fail(); @@ -1061,7 +1062,7 @@ public void testPaddedPartition_empty() { } public void testPaddedPartition_singleton1() { - Iterator source = singletonIterator(1); + Iterator source = Iterators.singletonIterator(1); Iterator> partitions = Iterators.paddedPartition(source, 1); assertTrue(partitions.hasNext()); assertTrue(partitions.hasNext()); @@ -1070,7 +1071,7 @@ public void testPaddedPartition_singleton1() { } public void testPaddedPartition_singleton2() { - Iterator source = singletonIterator(1); + Iterator source = Iterators.singletonIterator(1); Iterator> partitions = Iterators.paddedPartition(source, 2); assertTrue(partitions.hasNext()); assertTrue(partitions.hasNext()); @@ -1536,38 +1537,13 @@ public void testFrequency() { assertEquals(3, Iterators.frequency(list.iterator(), null)); } - public void testSingletonIteratorBasic() { - Iterator i = singletonIterator(1); - assertThat(i.hasNext()).isTrue(); - assertThat(i.next()).isEqualTo(1); - assertThat(i.hasNext()).isFalse(); - } - - public void testSingletonNullIteratorBasic() { - Iterator<@Nullable Integer> i = singletonIterator(null); - assertThat(i.hasNext()).isTrue(); - assertThat(i.next()).isEqualTo(null); - assertThat(i.hasNext()).isFalse(); - } - @GwtIncompatible // slow (~4s) - public void testSingletonIteratorWithIteratorTester() { + public void testSingletonIterator() { new IteratorTester( 3, UNMODIFIABLE, singleton(1), IteratorTester.KnownOrder.KNOWN_ORDER) { @Override protected Iterator newTargetIterator() { - return singletonIterator(1); - } - }.test(); - } - - @GwtIncompatible // slow (~4s) - public void testSingletonNullIteratorWithIteratorTester() { - new IteratorTester<@Nullable Integer>( - 3, UNMODIFIABLE, singleton(null), IteratorTester.KnownOrder.KNOWN_ORDER) { - @Override - protected Iterator<@Nullable Integer> newTargetIterator() { - return singletonIterator(null); + return Iterators.singletonIterator(1); } }.test(); } diff --git a/android/guava/src/com/google/common/collect/Iterators.java b/android/guava/src/com/google/common/collect/Iterators.java index 6cbb2f17ca5d..3b4a2b293a53 100644 --- a/android/guava/src/com/google/common/collect/Iterators.java +++ b/android/guava/src/com/google/common/collect/Iterators.java @@ -1100,54 +1100,34 @@ protected T get(int index) { */ public static UnmodifiableIterator singletonIterator( @ParametricNullness T value) { - if (value != null) { - return new SingletonIterator<>(value); - } - @SuppressWarnings("nullness") // For `value` to be null, T must be a nullable type. - UnmodifiableIterator result = (UnmodifiableIterator) new SingletonNullIterator(); - return result; + return new SingletonIterator<>(value); } private static final class SingletonIterator extends UnmodifiableIterator { - private @Nullable T valueOrNull; + private @Nullable Object valueOrThis; - SingletonIterator(@NonNull T value) { - this.valueOrNull = value; + SingletonIterator(T value) { + this.valueOrThis = value; } @Override public boolean hasNext() { - return valueOrNull != null; + return valueOrThis != this; } @Override - public @NonNull T next() { - T result = valueOrNull; - valueOrNull = null; + @ParametricNullness + public T next() { + Object result = valueOrThis; + valueOrThis = this; // We put the common case first, even though it's unlikely to matter if the code is run much: // https://shipilev.net/jvm/anatomy-quarks/28-frequency-based-code-layout/ - if (result != null) { - return result; - } - throw new NoSuchElementException(); - } - } - - private static final class SingletonNullIterator extends UnmodifiableIterator<@Nullable T> { - private boolean returned; - - @Override - public boolean hasNext() { - return !returned; - } - - @Override - public @Nullable T next() { - if (!returned) { - // common case first, as in SingletonIterator - returned = true; - return null; + if (result != this) { + // The field held either a `T` or `this`, and it turned out not to be `this`. + @SuppressWarnings("unchecked") + T t = (T) result; + return t; } throw new NoSuchElementException(); } diff --git a/guava-tests/test/com/google/common/collect/IteratorsTest.java b/guava-tests/test/com/google/common/collect/IteratorsTest.java index 837e64eebda5..7e173ab6a0bd 100644 --- a/guava-tests/test/com/google/common/collect/IteratorsTest.java +++ b/guava-tests/test/com/google/common/collect/IteratorsTest.java @@ -20,7 +20,6 @@ import static com.google.common.collect.Iterators.advance; import static com.google.common.collect.Iterators.get; import static com.google.common.collect.Iterators.getLast; -import static com.google.common.collect.Iterators.singletonIterator; import static com.google.common.collect.Lists.newArrayList; import static com.google.common.collect.testing.IteratorFeature.MODIFIABLE; import static com.google.common.collect.testing.IteratorFeature.UNMODIFIABLE; @@ -801,19 +800,21 @@ protected Iterator newTargetIterator() { } public void testConcatPartiallyAdvancedSecond() { - Iterator itr1 = Iterators.concat(singletonIterator("a"), Iterators.forArray("b", "c")); + Iterator itr1 = + Iterators.concat(Iterators.singletonIterator("a"), Iterators.forArray("b", "c")); assertEquals("a", itr1.next()); assertEquals("b", itr1.next()); - Iterator itr2 = Iterators.concat(singletonIterator("d"), itr1); + Iterator itr2 = Iterators.concat(Iterators.singletonIterator("d"), itr1); assertEquals("d", itr2.next()); assertEquals("c", itr2.next()); } public void testConcatPartiallyAdvancedFirst() { - Iterator itr1 = Iterators.concat(singletonIterator("a"), Iterators.forArray("b", "c")); + Iterator itr1 = + Iterators.concat(Iterators.singletonIterator("a"), Iterators.forArray("b", "c")); assertEquals("a", itr1.next()); assertEquals("b", itr1.next()); - Iterator itr2 = Iterators.concat(itr1, singletonIterator("d")); + Iterator itr2 = Iterators.concat(itr1, Iterators.singletonIterator("d")); assertEquals("c", itr2.next()); assertEquals("d", itr2.next()); } @@ -975,7 +976,7 @@ public void testElementsEqual() { } public void testPartition_badSize() { - Iterator source = singletonIterator(1); + Iterator source = Iterators.singletonIterator(1); try { Iterators.partition(source, 0); fail(); @@ -990,7 +991,7 @@ public void testPartition_empty() { } public void testPartition_singleton1() { - Iterator source = singletonIterator(1); + Iterator source = Iterators.singletonIterator(1); Iterator> partitions = Iterators.partition(source, 1); assertTrue(partitions.hasNext()); assertTrue(partitions.hasNext()); @@ -999,7 +1000,7 @@ public void testPartition_singleton1() { } public void testPartition_singleton2() { - Iterator source = singletonIterator(1); + Iterator source = Iterators.singletonIterator(1); Iterator> partitions = Iterators.partition(source, 2); assertTrue(partitions.hasNext()); assertTrue(partitions.hasNext()); @@ -1046,7 +1047,7 @@ public void testPartitionRandomAccess() { } public void testPaddedPartition_badSize() { - Iterator source = singletonIterator(1); + Iterator source = Iterators.singletonIterator(1); try { Iterators.paddedPartition(source, 0); fail(); @@ -1061,7 +1062,7 @@ public void testPaddedPartition_empty() { } public void testPaddedPartition_singleton1() { - Iterator source = singletonIterator(1); + Iterator source = Iterators.singletonIterator(1); Iterator> partitions = Iterators.paddedPartition(source, 1); assertTrue(partitions.hasNext()); assertTrue(partitions.hasNext()); @@ -1070,7 +1071,7 @@ public void testPaddedPartition_singleton1() { } public void testPaddedPartition_singleton2() { - Iterator source = singletonIterator(1); + Iterator source = Iterators.singletonIterator(1); Iterator> partitions = Iterators.paddedPartition(source, 2); assertTrue(partitions.hasNext()); assertTrue(partitions.hasNext()); @@ -1536,38 +1537,13 @@ public void testFrequency() { assertEquals(3, Iterators.frequency(list.iterator(), null)); } - public void testSingletonIteratorBasic() { - Iterator i = singletonIterator(1); - assertThat(i.hasNext()).isTrue(); - assertThat(i.next()).isEqualTo(1); - assertThat(i.hasNext()).isFalse(); - } - - public void testSingletonNullIteratorBasic() { - Iterator<@Nullable Integer> i = singletonIterator(null); - assertThat(i.hasNext()).isTrue(); - assertThat(i.next()).isEqualTo(null); - assertThat(i.hasNext()).isFalse(); - } - @GwtIncompatible // slow (~4s) - public void testSingletonIteratorWithIteratorTester() { + public void testSingletonIterator() { new IteratorTester( 3, UNMODIFIABLE, singleton(1), IteratorTester.KnownOrder.KNOWN_ORDER) { @Override protected Iterator newTargetIterator() { - return singletonIterator(1); - } - }.test(); - } - - @GwtIncompatible // slow (~4s) - public void testSingletonNullIteratorWithIteratorTester() { - new IteratorTester<@Nullable Integer>( - 3, UNMODIFIABLE, singleton(null), IteratorTester.KnownOrder.KNOWN_ORDER) { - @Override - protected Iterator<@Nullable Integer> newTargetIterator() { - return singletonIterator(null); + return Iterators.singletonIterator(1); } }.test(); } diff --git a/guava/src/com/google/common/collect/Iterators.java b/guava/src/com/google/common/collect/Iterators.java index 6cbb2f17ca5d..3b4a2b293a53 100644 --- a/guava/src/com/google/common/collect/Iterators.java +++ b/guava/src/com/google/common/collect/Iterators.java @@ -1100,54 +1100,34 @@ protected T get(int index) { */ public static UnmodifiableIterator singletonIterator( @ParametricNullness T value) { - if (value != null) { - return new SingletonIterator<>(value); - } - @SuppressWarnings("nullness") // For `value` to be null, T must be a nullable type. - UnmodifiableIterator result = (UnmodifiableIterator) new SingletonNullIterator(); - return result; + return new SingletonIterator<>(value); } private static final class SingletonIterator extends UnmodifiableIterator { - private @Nullable T valueOrNull; + private @Nullable Object valueOrThis; - SingletonIterator(@NonNull T value) { - this.valueOrNull = value; + SingletonIterator(T value) { + this.valueOrThis = value; } @Override public boolean hasNext() { - return valueOrNull != null; + return valueOrThis != this; } @Override - public @NonNull T next() { - T result = valueOrNull; - valueOrNull = null; + @ParametricNullness + public T next() { + Object result = valueOrThis; + valueOrThis = this; // We put the common case first, even though it's unlikely to matter if the code is run much: // https://shipilev.net/jvm/anatomy-quarks/28-frequency-based-code-layout/ - if (result != null) { - return result; - } - throw new NoSuchElementException(); - } - } - - private static final class SingletonNullIterator extends UnmodifiableIterator<@Nullable T> { - private boolean returned; - - @Override - public boolean hasNext() { - return !returned; - } - - @Override - public @Nullable T next() { - if (!returned) { - // common case first, as in SingletonIterator - returned = true; - return null; + if (result != this) { + // The field held either a `T` or `this`, and it turned out not to be `this`. + @SuppressWarnings("unchecked") + T t = (T) result; + return t; } throw new NoSuchElementException(); }