Skip to content

Commit

Permalink
Further micro-optimize singletonIterator, just to be safe / waste m…
Browse files Browse the repository at this point in the history
…y morning.

After cl/591014189, we're seeing the expected (tiny) memory improvements. (It's also possible that we're seeing other memory improvements from clearing references earlier, but that is speculative.)

However, we're also seeing higher CPU numbers. I kind of expect to learn that that's a profiling artifact. (Microbenchmarks show an _improvement_ from that CL.) But just in case it's a result of having to read `static final` fields, let's stop doing that. (Also, if it matters, this CL ends up making the bytecode of `next` and `hasNext` _almost_ as short as it was before cl/591014189.)

I could imagine trying yet further things, like moving the `throw` statement into a helper method to reduce the size of `SingletonIterator.next()` itself, perhaps helping inlining (even though the method is already below the `MaxInlineSize`, which is normally 35 per `java -XX:+PrintFlagsFinal`). (Benchmarks didn't show any improvement, but I don't know how well they'd reflect inlining, especially multiple rounds of inlining? My memory of all this is fuzzy.) Mainly, though, if there is anything that actually needs to be done after cl/591014189, it's probably this CL. And we likely don't really need even that.

(I had also considered a different approach to this CL that used `null` instead of `this`: cl/626382992. However, that required a second implementation class for the null case (or another `static final` sentinel), and I worry that that may make iteration over `ImmutableList`, etc. become megamorphic for some callers. (If anything, maybe we should go the opposite direction and introduce another common iterator superclass with an index and size in it (or just `remainingElements`??) so that it can implement `hasNext` for both singleton and non-singleton iterators!))

(Yet another possibility would be to keep the sentinel but make it of a special type so that I can use `instanceof`. But everything that I hear about https://github.com/RedHatPerf/type-pollution-agent suggests to me that calling `instanceof` on random objects is not a good way to improve performance :))

RELNOTES=n/a
PiperOrigin-RevId: 626542813
  • Loading branch information
cpovirk authored and Google Java Core Libraries committed Apr 20, 2024
1 parent 1792bff commit 183837d
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 24 deletions.
25 changes: 13 additions & 12 deletions android/guava/src/com/google/common/collect/Iterators.java
Expand Up @@ -1105,30 +1105,31 @@ protected T get(int index) {

private static final class SingletonIterator<T extends @Nullable Object>
extends UnmodifiableIterator<T> {
private static final Object SENTINEL = new Object();

private @Nullable Object valueOrSentinel;
private @Nullable Object valueOrThis;

SingletonIterator(T value) {
this.valueOrSentinel = value;
this.valueOrThis = value;
}

@Override
public boolean hasNext() {
return valueOrSentinel != SENTINEL;
return valueOrThis != this;
}

@Override
@ParametricNullness
public T next() {
if (valueOrSentinel == SENTINEL) {
throw new NoSuchElementException();
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 != 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;
}
// The field held either a T or SENTINEL, and it turned out not to be SENTINEL.
@SuppressWarnings("unchecked")
T t = (T) valueOrSentinel;
valueOrSentinel = SENTINEL;
return t;
throw new NoSuchElementException();
}
}

Expand Down
25 changes: 13 additions & 12 deletions guava/src/com/google/common/collect/Iterators.java
Expand Up @@ -1105,30 +1105,31 @@ protected T get(int index) {

private static final class SingletonIterator<T extends @Nullable Object>
extends UnmodifiableIterator<T> {
private static final Object SENTINEL = new Object();

private @Nullable Object valueOrSentinel;
private @Nullable Object valueOrThis;

SingletonIterator(T value) {
this.valueOrSentinel = value;
this.valueOrThis = value;
}

@Override
public boolean hasNext() {
return valueOrSentinel != SENTINEL;
return valueOrThis != this;
}

@Override
@ParametricNullness
public T next() {
if (valueOrSentinel == SENTINEL) {
throw new NoSuchElementException();
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 != 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;
}
// The field held either a T or SENTINEL, and it turned out not to be SENTINEL.
@SuppressWarnings("unchecked")
T t = (T) valueOrSentinel;
valueOrSentinel = SENTINEL;
return t;
throw new NoSuchElementException();
}
}

Expand Down

0 comments on commit 183837d

Please sign in to comment.