aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorcpovirk <cpovirk@google.com>2024-04-19 20:30:05 -0700
committerGoogle Java Core Libraries <jake-team+copybara@google.com>2024-04-19 20:33:06 -0700
commit183837d2f3e62c24af386e808071aced230b942c (patch)
treeb7472bd654eb939be5c7234a461345a825344e53
parent1792bffe772f9ba2287a5f1e49d965b5ab27e151 (diff)
downloadguava-183837d2f3e62c24af386e808071aced230b942c.tar.gz
Further micro-optimize `singletonIterator`, just to be safe / waste my 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
-rw-r--r--android/guava/src/com/google/common/collect/Iterators.java25
-rw-r--r--guava/src/com/google/common/collect/Iterators.java25
2 files changed, 26 insertions, 24 deletions
diff --git a/android/guava/src/com/google/common/collect/Iterators.java b/android/guava/src/com/google/common/collect/Iterators.java
index 9f7b6f711..3b4a2b293 100644
--- a/android/guava/src/com/google/common/collect/Iterators.java
+++ b/android/guava/src/com/google/common/collect/Iterators.java
@@ -1105,30 +1105,31 @@ public final class Iterators {
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();
}
}
diff --git a/guava/src/com/google/common/collect/Iterators.java b/guava/src/com/google/common/collect/Iterators.java
index 9f7b6f711..3b4a2b293 100644
--- a/guava/src/com/google/common/collect/Iterators.java
+++ b/guava/src/com/google/common/collect/Iterators.java
@@ -1105,30 +1105,31 @@ public final class Iterators {
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();
}
}