From 400af25292096746ed3f6164f0ff88209acbb19f Mon Sep 17 00:00:00 2001 From: cpovirk Date: Tue, 7 Jan 2025 12:40:00 -0800 Subject: [PATCH] Make `AtomicReferenceFieldUpdater` fields `static` for [better performance](https://shipilev.net/blog/2015/faster-atomic-fu/#:~:text=thrown%20out%20of%20the%20window). This may eliminate the reason for [an `Unsafe`-based implementation](https://github.com/google/guava/issues/6806) even under Java 8, but we will ideally confirm that with benchmarks before ripping that implementation out. (There's also Android: `AtomicReferenceFieldUpdater` is available there, but we should look into performance, including under older versions, especially with AFAIK no plans to remove `Unsafe` there.) (Also, make a few other tiny edits to the backport copy so that it matches the mainline copy more closely.) RELNOTES=n/a PiperOrigin-RevId: 713006636 --- ...teFutureStateFallbackAtomicHelperTest.java | 8 ++- .../util/concurrent/AbstractFuture.java | 65 ++++++++++--------- ...teFutureStateFallbackAtomicHelperTest.java | 8 ++- .../util/concurrent/AbstractFuture.java | 59 ++++++++--------- 4 files changed, 77 insertions(+), 63 deletions(-) diff --git a/android/guava-tests/test/com/google/common/util/concurrent/AggregateFutureStateFallbackAtomicHelperTest.java b/android/guava-tests/test/com/google/common/util/concurrent/AggregateFutureStateFallbackAtomicHelperTest.java index 1ed1d10d77d6..9823f5fcc8d7 100644 --- a/android/guava-tests/test/com/google/common/util/concurrent/AggregateFutureStateFallbackAtomicHelperTest.java +++ b/android/guava-tests/test/com/google/common/util/concurrent/AggregateFutureStateFallbackAtomicHelperTest.java @@ -68,7 +68,13 @@ public static TestSuite suite() { // corresponding method on FuturesTest in the correct classloader. TestSuite suite = new TestSuite(AggregateFutureStateFallbackAtomicHelperTest.class.getName()); for (Method method : FuturesTest.class.getDeclaredMethods()) { - if (Modifier.isPublic(method.getModifiers()) && method.getName().startsWith("test")) { + if (Modifier.isPublic(method.getModifiers()) + && method.getName().startsWith("test") + /* + * When we block access to AtomicReferenceFieldUpdater, we can't even reflect on + * AbstractFuture, since it declares methods that use that type in their signatures. + */ + && !method.getName().equals("testFutures_nullChecks")) { suite.addTest( TestSuite.createTest( AggregateFutureStateFallbackAtomicHelperTest.class, method.getName())); diff --git a/android/guava/src/com/google/common/util/concurrent/AbstractFuture.java b/android/guava/src/com/google/common/util/concurrent/AbstractFuture.java index 1e3942fe0def..4b119e03b162 100644 --- a/android/guava/src/com/google/common/util/concurrent/AbstractFuture.java +++ b/android/guava/src/com/google/common/util/concurrent/AbstractFuture.java @@ -159,24 +159,15 @@ public final boolean cancel(boolean mayInterruptIfRunning) { helper = new UnsafeAtomicHelper(); } catch (Exception | Error unsafeFailure) { // sneaky checked exception thrownUnsafeFailure = unsafeFailure; - // catch absolutely everything and fall through to our - // 'AtomicReferenceFieldUpdaterAtomicHelper' The access control checks that ARFU does means - // the caller class has to be AbstractFuture instead of - // AtomicReferenceFieldUpdaterAtomicHelper, so we annoyingly define these here + // Catch absolutely everything and fall through to AtomicReferenceFieldUpdaterAtomicHelper. try { - helper = - new AtomicReferenceFieldUpdaterAtomicHelper( - newUpdater(Waiter.class, Thread.class, "thread"), - newUpdater(Waiter.class, Waiter.class, "next"), - newUpdater(AbstractFuture.class, Waiter.class, "waiters"), - newUpdater(AbstractFuture.class, Listener.class, "listeners"), - newUpdater(AbstractFuture.class, Object.class, "value")); + helper = new AtomicReferenceFieldUpdaterAtomicHelper(); } catch (Exception // sneaky checked exception | Error atomicReferenceFieldUpdaterFailure) { // Some Android 5.0.x Samsung devices have bugs in JDK reflection APIs that cause // getDeclaredField to throw a NoSuchFieldException when the field is definitely there. - // For these users fallback to a suboptimal implementation, based on synchronized. This will - // be a definite performance hit to those users. + // For these users fallback to a suboptimal implementation, based on synchronized. This + // will be a definite performance hit to those users. thrownAtomicReferenceFieldUpdaterFailure = atomicReferenceFieldUpdaterFailure; helper = new SynchronizedHelper(); } @@ -716,7 +707,7 @@ mayInterruptIfRunning, new CancellationException("Future.cancel() was called.")) * *

The default implementation does nothing. * - *

This method is likely to be deprecated. Prefer to override {@link #afterDone}, consulting + *

This method is likely to be deprecated. Prefer to override {@link #afterDone}, checking * {@link #wasInterrupted} to decide whether to interrupt your task. * * @since 10.0 @@ -1447,24 +1438,16 @@ boolean casValue(AbstractFuture future, @Nullable Object expect, Object updat /** {@link AtomicHelper} based on {@link AtomicReferenceFieldUpdater}. */ private static final class AtomicReferenceFieldUpdaterAtomicHelper extends AtomicHelper { - final AtomicReferenceFieldUpdater waiterThreadUpdater; - final AtomicReferenceFieldUpdater waiterNextUpdater; - final AtomicReferenceFieldUpdater, Waiter> waitersUpdater; - final AtomicReferenceFieldUpdater, Listener> listenersUpdater; - final AtomicReferenceFieldUpdater, Object> valueUpdater; - - AtomicReferenceFieldUpdaterAtomicHelper( - AtomicReferenceFieldUpdater waiterThreadUpdater, - AtomicReferenceFieldUpdater waiterNextUpdater, - AtomicReferenceFieldUpdater, Waiter> waitersUpdater, - AtomicReferenceFieldUpdater, Listener> listenersUpdater, - AtomicReferenceFieldUpdater, Object> valueUpdater) { - this.waiterThreadUpdater = waiterThreadUpdater; - this.waiterNextUpdater = waiterNextUpdater; - this.waitersUpdater = waitersUpdater; - this.listenersUpdater = listenersUpdater; - this.valueUpdater = valueUpdater; - } + private static final AtomicReferenceFieldUpdater waiterThreadUpdater = + newUpdater(Waiter.class, Thread.class, "thread"); + private static final AtomicReferenceFieldUpdater waiterNextUpdater = + newUpdater(Waiter.class, Waiter.class, "next"); + private static final AtomicReferenceFieldUpdater, Waiter> + waitersUpdater = waitersUpdaterFromWithinAbstractFuture(); + private static final AtomicReferenceFieldUpdater, Listener> + listenersUpdater = listenersUpdaterFromWithinAbstractFuture(); + private static final AtomicReferenceFieldUpdater, Object> + valueUpdater = valueUpdaterFromWithinAbstractFuture(); @Override void putThread(Waiter waiter, Thread newValue) { @@ -1502,6 +1485,24 @@ boolean casValue(AbstractFuture future, @Nullable Object expect, Object updat } } + /** Returns an {@link AtomicReferenceFieldUpdater} for {@link #waiters}. */ + private static AtomicReferenceFieldUpdater, Waiter> + waitersUpdaterFromWithinAbstractFuture() { + return newUpdater(AbstractFuture.class, Waiter.class, "waiters"); + } + + /** Returns an {@link AtomicReferenceFieldUpdater} for {@link #listeners}. */ + private static AtomicReferenceFieldUpdater, Listener> + listenersUpdaterFromWithinAbstractFuture() { + return newUpdater(AbstractFuture.class, Listener.class, "listeners"); + } + + /** Returns an {@link AtomicReferenceFieldUpdater} for {@link #value}. */ + private static AtomicReferenceFieldUpdater, Object> + valueUpdaterFromWithinAbstractFuture() { + return newUpdater(AbstractFuture.class, Object.class, "value"); + } + /** * {@link AtomicHelper} based on {@code synchronized} and volatile writes. * diff --git a/guava-tests/test/com/google/common/util/concurrent/AggregateFutureStateFallbackAtomicHelperTest.java b/guava-tests/test/com/google/common/util/concurrent/AggregateFutureStateFallbackAtomicHelperTest.java index 1ed1d10d77d6..9823f5fcc8d7 100644 --- a/guava-tests/test/com/google/common/util/concurrent/AggregateFutureStateFallbackAtomicHelperTest.java +++ b/guava-tests/test/com/google/common/util/concurrent/AggregateFutureStateFallbackAtomicHelperTest.java @@ -68,7 +68,13 @@ public static TestSuite suite() { // corresponding method on FuturesTest in the correct classloader. TestSuite suite = new TestSuite(AggregateFutureStateFallbackAtomicHelperTest.class.getName()); for (Method method : FuturesTest.class.getDeclaredMethods()) { - if (Modifier.isPublic(method.getModifiers()) && method.getName().startsWith("test")) { + if (Modifier.isPublic(method.getModifiers()) + && method.getName().startsWith("test") + /* + * When we block access to AtomicReferenceFieldUpdater, we can't even reflect on + * AbstractFuture, since it declares methods that use that type in their signatures. + */ + && !method.getName().equals("testFutures_nullChecks")) { suite.addTest( TestSuite.createTest( AggregateFutureStateFallbackAtomicHelperTest.class, method.getName())); diff --git a/guava/src/com/google/common/util/concurrent/AbstractFuture.java b/guava/src/com/google/common/util/concurrent/AbstractFuture.java index ad3cf5471ad4..02d8aa94bfa6 100644 --- a/guava/src/com/google/common/util/concurrent/AbstractFuture.java +++ b/guava/src/com/google/common/util/concurrent/AbstractFuture.java @@ -186,18 +186,9 @@ private static MethodHandles.Lookup methodHandlesLookupFromWithinAbstractFuture( helper = new UnsafeAtomicHelper(); } catch (Exception | Error unsafeFailure) { // sneaky checked exception thrownUnsafeFailure = unsafeFailure; - // catch absolutely everything and fall through to our - // 'AtomicReferenceFieldUpdaterAtomicHelper' The access control checks that ARFU does means - // the caller class has to be AbstractFuture instead of - // AtomicReferenceFieldUpdaterAtomicHelper, so we annoyingly define these here + // Catch absolutely everything and fall through to AtomicReferenceFieldUpdaterAtomicHelper. try { - helper = - new AtomicReferenceFieldUpdaterAtomicHelper( - newUpdater(Waiter.class, Thread.class, "thread"), - newUpdater(Waiter.class, Waiter.class, "next"), - newUpdater(AbstractFuture.class, Waiter.class, "waiters"), - newUpdater(AbstractFuture.class, Listener.class, "listeners"), - newUpdater(AbstractFuture.class, Object.class, "value")); + helper = new AtomicReferenceFieldUpdaterAtomicHelper(); } catch (Exception // sneaky checked exception | Error atomicReferenceFieldUpdaterFailure) { // Some Android 5.0.x Samsung devices have bugs in JDK reflection APIs that cause @@ -1559,24 +1550,16 @@ boolean casValue(AbstractFuture future, @Nullable Object expect, Object updat /** {@link AtomicHelper} based on {@link AtomicReferenceFieldUpdater}. */ private static final class AtomicReferenceFieldUpdaterAtomicHelper extends AtomicHelper { - final AtomicReferenceFieldUpdater waiterThreadUpdater; - final AtomicReferenceFieldUpdater waiterNextUpdater; - final AtomicReferenceFieldUpdater, Waiter> waitersUpdater; - final AtomicReferenceFieldUpdater, Listener> listenersUpdater; - final AtomicReferenceFieldUpdater, Object> valueUpdater; - - AtomicReferenceFieldUpdaterAtomicHelper( - AtomicReferenceFieldUpdater waiterThreadUpdater, - AtomicReferenceFieldUpdater waiterNextUpdater, - AtomicReferenceFieldUpdater, Waiter> waitersUpdater, - AtomicReferenceFieldUpdater, Listener> listenersUpdater, - AtomicReferenceFieldUpdater, Object> valueUpdater) { - this.waiterThreadUpdater = waiterThreadUpdater; - this.waiterNextUpdater = waiterNextUpdater; - this.waitersUpdater = waitersUpdater; - this.listenersUpdater = listenersUpdater; - this.valueUpdater = valueUpdater; - } + private static final AtomicReferenceFieldUpdater waiterThreadUpdater = + newUpdater(Waiter.class, Thread.class, "thread"); + private static final AtomicReferenceFieldUpdater waiterNextUpdater = + newUpdater(Waiter.class, Waiter.class, "next"); + private static final AtomicReferenceFieldUpdater, Waiter> + waitersUpdater = waitersUpdaterFromWithinAbstractFuture(); + private static final AtomicReferenceFieldUpdater, Listener> + listenersUpdater = listenersUpdaterFromWithinAbstractFuture(); + private static final AtomicReferenceFieldUpdater, Object> + valueUpdater = valueUpdaterFromWithinAbstractFuture(); @Override void putThread(Waiter waiter, Thread newValue) { @@ -1614,6 +1597,24 @@ boolean casValue(AbstractFuture future, @Nullable Object expect, Object updat } } + /** Returns an {@link AtomicReferenceFieldUpdater} for {@link #waiters}. */ + private static AtomicReferenceFieldUpdater, Waiter> + waitersUpdaterFromWithinAbstractFuture() { + return newUpdater(AbstractFuture.class, Waiter.class, "waiters"); + } + + /** Returns an {@link AtomicReferenceFieldUpdater} for {@link #listeners}. */ + private static AtomicReferenceFieldUpdater, Listener> + listenersUpdaterFromWithinAbstractFuture() { + return newUpdater(AbstractFuture.class, Listener.class, "listeners"); + } + + /** Returns an {@link AtomicReferenceFieldUpdater} for {@link #value}. */ + private static AtomicReferenceFieldUpdater, Object> + valueUpdaterFromWithinAbstractFuture() { + return newUpdater(AbstractFuture.class, Object.class, "value"); + } + /** * {@link AtomicHelper} based on {@code synchronized} and volatile writes. *