Skip to content

Commit

Permalink
Roll back the most recent singletonIterator optimization attempt.
Browse files Browse the repository at this point in the history 
(I'll probably just roll all my attempts back.)

We got reports of a regression in Bazel.

PiperOrigin-RevId: 630366325
  • Loading branch information
@cpovirk
cpovirk authored and Google Java Core Libraries committed May 3, 2024
1 parent 57f76e3 commit 2f4154d
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 144 deletions.
52 changes: 14 additions & 38 deletions android/guava-tests/test/com/google/common/collect/IteratorsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -801,19 +800,21 @@ protected Iterator<Integer> newTargetIterator() {
}

public void testConcatPartiallyAdvancedSecond() {
Iterator<String> itr1 = Iterators.concat(singletonIterator("a"), Iterators.forArray("b", "c"));
Iterator<String> itr1 =
Iterators.concat(Iterators.singletonIterator("a"), Iterators.forArray("b", "c"));
assertEquals("a", itr1.next());
assertEquals("b", itr1.next());
Iterator<String> itr2 = Iterators.concat(singletonIterator("d"), itr1);
Iterator<String> itr2 = Iterators.concat(Iterators.singletonIterator("d"), itr1);
assertEquals("d", itr2.next());
assertEquals("c", itr2.next());
}

public void testConcatPartiallyAdvancedFirst() {
Iterator<String> itr1 = Iterators.concat(singletonIterator("a"), Iterators.forArray("b", "c"));
Iterator<String> itr1 =
Iterators.concat(Iterators.singletonIterator("a"), Iterators.forArray("b", "c"));
assertEquals("a", itr1.next());
assertEquals("b", itr1.next());
Iterator<String> itr2 = Iterators.concat(itr1, singletonIterator("d"));
Iterator<String> itr2 = Iterators.concat(itr1, Iterators.singletonIterator("d"));
assertEquals("c", itr2.next());
assertEquals("d", itr2.next());
}
Expand Down Expand Up @@ -975,7 +976,7 @@ public void testElementsEqual() {
}

public void testPartition_badSize() {
Iterator<Integer> source = singletonIterator(1);
Iterator<Integer> source = Iterators.singletonIterator(1);
try {
Iterators.partition(source, 0);
fail();
Expand All @@ -990,7 +991,7 @@ public void testPartition_empty() {
}

public void testPartition_singleton1() {
Iterator<Integer> source = singletonIterator(1);
Iterator<Integer> source = Iterators.singletonIterator(1);
Iterator<List<Integer>> partitions = Iterators.partition(source, 1);
assertTrue(partitions.hasNext());
assertTrue(partitions.hasNext());
Expand All @@ -999,7 +1000,7 @@ public void testPartition_singleton1() {
}

public void testPartition_singleton2() {
Iterator<Integer> source = singletonIterator(1);
Iterator<Integer> source = Iterators.singletonIterator(1);
Iterator<List<Integer>> partitions = Iterators.partition(source, 2);
assertTrue(partitions.hasNext());
assertTrue(partitions.hasNext());
Expand Down Expand Up @@ -1046,7 +1047,7 @@ public void testPartitionRandomAccess() {
}

public void testPaddedPartition_badSize() {
Iterator<Integer> source = singletonIterator(1);
Iterator<Integer> source = Iterators.singletonIterator(1);
try {
Iterators.paddedPartition(source, 0);
fail();
Expand All @@ -1061,7 +1062,7 @@ public void testPaddedPartition_empty() {
}

public void testPaddedPartition_singleton1() {
Iterator<Integer> source = singletonIterator(1);
Iterator<Integer> source = Iterators.singletonIterator(1);
Iterator<List<Integer>> partitions = Iterators.paddedPartition(source, 1);
assertTrue(partitions.hasNext());
assertTrue(partitions.hasNext());
Expand All @@ -1070,7 +1071,7 @@ public void testPaddedPartition_singleton1() {
}

public void testPaddedPartition_singleton2() {
Iterator<Integer> source = singletonIterator(1);
Iterator<Integer> source = Iterators.singletonIterator(1);
Iterator<List<Integer>> partitions = Iterators.paddedPartition(source, 2);
assertTrue(partitions.hasNext());
assertTrue(partitions.hasNext());
Expand Down Expand Up @@ -1536,38 +1537,13 @@ public void testFrequency() {
assertEquals(3, Iterators.frequency(list.iterator(), null));
}

public void testSingletonIteratorBasic() {
Iterator<Integer> 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<Integer>(
3, UNMODIFIABLE, singleton(1), IteratorTester.KnownOrder.KNOWN_ORDER) {
@Override
protected Iterator<Integer> 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();
}
Expand Down
48 changes: 14 additions & 34 deletions android/guava/src/com/google/common/collect/Iterators.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1100,54 +1100,34 @@ protected T get(int index) {
*/
public static <T extends @Nullable Object> UnmodifiableIterator<T> 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<T> result = (UnmodifiableIterator<T>) new SingletonNullIterator<T>();
return result;
return new SingletonIterator<>(value);
}

private static final class SingletonIterator<T extends @Nullable Object>
extends UnmodifiableIterator<T> {
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<T> 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();
}
Expand Down
52 changes: 14 additions & 38 deletions guava-tests/test/com/google/common/collect/IteratorsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -801,19 +800,21 @@ protected Iterator<Integer> newTargetIterator() {
}

public void testConcatPartiallyAdvancedSecond() {
Iterator<String> itr1 = Iterators.concat(singletonIterator("a"), Iterators.forArray("b", "c"));
Iterator<String> itr1 =
Iterators.concat(Iterators.singletonIterator("a"), Iterators.forArray("b", "c"));
assertEquals("a", itr1.next());
assertEquals("b", itr1.next());
Iterator<String> itr2 = Iterators.concat(singletonIterator("d"), itr1);
Iterator<String> itr2 = Iterators.concat(Iterators.singletonIterator("d"), itr1);
assertEquals("d", itr2.next());
assertEquals("c", itr2.next());
}

public void testConcatPartiallyAdvancedFirst() {
Iterator<String> itr1 = Iterators.concat(singletonIterator("a"), Iterators.forArray("b", "c"));
Iterator<String> itr1 =
Iterators.concat(Iterators.singletonIterator("a"), Iterators.forArray("b", "c"));
assertEquals("a", itr1.next());
assertEquals("b", itr1.next());
Iterator<String> itr2 = Iterators.concat(itr1, singletonIterator("d"));
Iterator<String> itr2 = Iterators.concat(itr1, Iterators.singletonIterator("d"));
assertEquals("c", itr2.next());
assertEquals("d", itr2.next());
}
Expand Down Expand Up @@ -975,7 +976,7 @@ public void testElementsEqual() {
}

public void testPartition_badSize() {
Iterator<Integer> source = singletonIterator(1);
Iterator<Integer> source = Iterators.singletonIterator(1);
try {
Iterators.partition(source, 0);
fail();
Expand All @@ -990,7 +991,7 @@ public void testPartition_empty() {
}

public void testPartition_singleton1() {
Iterator<Integer> source = singletonIterator(1);
Iterator<Integer> source = Iterators.singletonIterator(1);
Iterator<List<Integer>> partitions = Iterators.partition(source, 1);
assertTrue(partitions.hasNext());
assertTrue(partitions.hasNext());
Expand All @@ -999,7 +1000,7 @@ public void testPartition_singleton1() {
}

public void testPartition_singleton2() {
Iterator<Integer> source = singletonIterator(1);
Iterator<Integer> source = Iterators.singletonIterator(1);
Iterator<List<Integer>> partitions = Iterators.partition(source, 2);
assertTrue(partitions.hasNext());
assertTrue(partitions.hasNext());
Expand Down Expand Up @@ -1046,7 +1047,7 @@ public void testPartitionRandomAccess() {
}

public void testPaddedPartition_badSize() {
Iterator<Integer> source = singletonIterator(1);
Iterator<Integer> source = Iterators.singletonIterator(1);
try {
Iterators.paddedPartition(source, 0);
fail();
Expand All @@ -1061,7 +1062,7 @@ public void testPaddedPartition_empty() {
}

public void testPaddedPartition_singleton1() {
Iterator<Integer> source = singletonIterator(1);
Iterator<Integer> source = Iterators.singletonIterator(1);
Iterator<List<Integer>> partitions = Iterators.paddedPartition(source, 1);
assertTrue(partitions.hasNext());
assertTrue(partitions.hasNext());
Expand All @@ -1070,7 +1071,7 @@ public void testPaddedPartition_singleton1() {
}

public void testPaddedPartition_singleton2() {
Iterator<Integer> source = singletonIterator(1);
Iterator<Integer> source = Iterators.singletonIterator(1);
Iterator<List<Integer>> partitions = Iterators.paddedPartition(source, 2);
assertTrue(partitions.hasNext());
assertTrue(partitions.hasNext());
Expand Down Expand Up @@ -1536,38 +1537,13 @@ public void testFrequency() {
assertEquals(3, Iterators.frequency(list.iterator(), null));
}

public void testSingletonIteratorBasic() {
Iterator<Integer> 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<Integer>(
3, UNMODIFIABLE, singleton(1), IteratorTester.KnownOrder.KNOWN_ORDER) {
@Override
protected Iterator<Integer> 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();
}
Expand Down
48 changes: 14 additions & 34 deletions guava/src/com/google/common/collect/Iterators.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1100,54 +1100,34 @@ protected T get(int index) {
*/
public static <T extends @Nullable Object> UnmodifiableIterator<T> 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<T> result = (UnmodifiableIterator<T>) new SingletonNullIterator<T>();
return result;
return new SingletonIterator<>(value);
}

private static final class SingletonIterator<T extends @Nullable Object>
extends UnmodifiableIterator<T> {
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<T> 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();
}
Expand Down

0 comments on commit 2f4154d

Please sign in to comment.