From 7283b8b843434ff747802c919ecd245eed53ef6d Mon Sep 17 00:00:00 2001 From: cpovirk Date: Thu, 12 Dec 2024 12:11:27 -0800 Subject: [PATCH] Prefactoring for future changes that will avoid using `Unsafe`. See https://github.com/google/guava/issues/6806#issuecomment-2513902266. Changes: - "`SafeAtomicHelper`" is arguably already too generic a name for that class, given that we have a `SynchronizedAtomicHelper` that also avoids using `Unsafe`. It's going to become even more overly generic (and more overly scary) when we likely introduce a `VarHandle`-based alternative. (And maybe we'll even remove the `Unsafe`-based one entirely?) Rename it. - Remove Javadoc from implementation classes, since it merely duplicates that from the superclass. - Fix links in the (package-private) Javadoc. I considered also renaming the `AtomicHelper` methods to match the terminology of `VarHandle`. This would mean only renaming `putThread`+`putNext` to... `setReleaseThread`? `setThreadReleasedly`? `setThreadUsingReleaseAccessMode`? I didn't find anything that I particularly liked. RELNOTES=n/a PiperOrigin-RevId: 705587524 --- ...bstractFutureFallbackAtomicHelperTest.java | 2 +- .../util/concurrent/AbstractFuture.java | 34 +++++++------------ ...bstractFutureFallbackAtomicHelperTest.java | 2 +- .../util/concurrent/AbstractFuture.java | 34 +++++++------------ 4 files changed, 28 insertions(+), 44 deletions(-) diff --git a/android/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java b/android/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java index 42cb8a1fbecc..f106dee2e074 100644 --- a/android/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java +++ b/android/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java @@ -85,7 +85,7 @@ public static TestSuite suite() { public void runTest() throws Exception { // First ensure that our classloaders are initializing the correct helper versions checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper"); - checkHelperVersion(NO_UNSAFE, "SafeAtomicHelper"); + checkHelperVersion(NO_UNSAFE, "AtomicReferenceFieldUpdaterAtomicHelper"); checkHelperVersion(NO_ATOMIC_REFERENCE_FIELD_UPDATER, "SynchronizedHelper"); // Run the corresponding AbstractFutureTest test method in a new classloader that disallows 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 477abc412b92..409ab362b284 100644 --- a/android/guava/src/com/google/common/util/concurrent/AbstractFuture.java +++ b/android/guava/src/com/google/common/util/concurrent/AbstractFuture.java @@ -161,12 +161,13 @@ 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 'SafeAtomicHelper' - // The access control checks that ARFU does means the caller class has to be AbstractFuture - // instead of SafeAtomicHelper, so we annoyingly define these here + // 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 try { helper = - new SafeAtomicHelper( + new AtomicReferenceFieldUpdaterAtomicHelper( newUpdater(Waiter.class, Thread.class, "thread"), newUpdater(Waiter.class, Waiter.class, "next"), newUpdater(AbstractFuture.class, Waiter.class, "waiters"), @@ -196,7 +197,7 @@ public final boolean cancel(boolean mayInterruptIfRunning) { log.get() .log( Level.SEVERE, - "SafeAtomicHelper is broken!", + "AtomicReferenceFieldUpdaterAtomicHelper is broken!", thrownAtomicReferenceFieldUpdaterFailure); } } @@ -1325,21 +1326,21 @@ private abstract static class AtomicHelper { /** Non-volatile write of the waiter to the {@link Waiter#next} field. */ abstract void putNext(Waiter waiter, @CheckForNull Waiter newValue); - /** Performs a CAS operation on the {@link #waiters} field. */ + /** Performs a CAS operation on the {@link AbstractFuture#waiters} field. */ abstract boolean casWaiters( AbstractFuture future, @CheckForNull Waiter expect, @CheckForNull Waiter update); - /** Performs a CAS operation on the {@link #listeners} field. */ + /** Performs a CAS operation on the {@link AbstractFuture#listeners} field. */ abstract boolean casListeners( AbstractFuture future, @CheckForNull Listener expect, Listener update); - /** Performs a GAS operation on the {@link #waiters} field. */ + /** Performs a GAS operation on the {@link AbstractFuture#waiters} field. */ abstract Waiter gasWaiters(AbstractFuture future, Waiter update); - /** Performs a GAS operation on the {@link #listeners} field. */ + /** Performs a GAS operation on the {@link AbstractFuture#listeners} field. */ abstract Listener gasListeners(AbstractFuture future, Listener update); - /** Performs a CAS operation on the {@link #value} field. */ + /** Performs a CAS operation on the {@link AbstractFuture#value} field. */ abstract boolean casValue(AbstractFuture future, @CheckForNull Object expect, Object update); } @@ -1407,20 +1408,17 @@ void putNext(Waiter waiter, @CheckForNull Waiter newValue) { UNSAFE.putObject(waiter, WAITER_NEXT_OFFSET, newValue); } - /** Performs a CAS operation on the {@link #waiters} field. */ @Override boolean casWaiters( AbstractFuture future, @CheckForNull Waiter expect, @CheckForNull Waiter update) { return UNSAFE.compareAndSwapObject(future, WAITERS_OFFSET, expect, update); } - /** Performs a CAS operation on the {@link #listeners} field. */ @Override boolean casListeners(AbstractFuture future, @CheckForNull Listener expect, Listener update) { return UNSAFE.compareAndSwapObject(future, LISTENERS_OFFSET, expect, update); } - /** Performs a GAS operation on the {@link #listeners} field. */ @Override Listener gasListeners(AbstractFuture future, Listener update) { while (true) { @@ -1434,7 +1432,6 @@ Listener gasListeners(AbstractFuture future, Listener update) { } } - /** Performs a GAS operation on the {@link #waiters} field. */ @Override Waiter gasWaiters(AbstractFuture future, Waiter update) { while (true) { @@ -1448,7 +1445,6 @@ Waiter gasWaiters(AbstractFuture future, Waiter update) { } } - /** Performs a CAS operation on the {@link #value} field. */ @Override boolean casValue(AbstractFuture future, @CheckForNull Object expect, Object update) { return UNSAFE.compareAndSwapObject(future, VALUE_OFFSET, expect, update); @@ -1456,14 +1452,14 @@ boolean casValue(AbstractFuture future, @CheckForNull Object expect, Object u } /** {@link AtomicHelper} based on {@link AtomicReferenceFieldUpdater}. */ - private static final class SafeAtomicHelper extends AtomicHelper { + 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; - SafeAtomicHelper( + AtomicReferenceFieldUpdaterAtomicHelper( AtomicReferenceFieldUpdater waiterThreadUpdater, AtomicReferenceFieldUpdater waiterNextUpdater, AtomicReferenceFieldUpdater, Waiter> waitersUpdater, @@ -1497,13 +1493,11 @@ boolean casListeners(AbstractFuture future, @CheckForNull Listener expect, Li return listenersUpdater.compareAndSet(future, expect, update); } - /** Performs a GAS operation on the {@link #listeners} field. */ @Override Listener gasListeners(AbstractFuture future, Listener update) { return listenersUpdater.getAndSet(future, update); } - /** Performs a GAS operation on the {@link #waiters} field. */ @Override Waiter gasWaiters(AbstractFuture future, Waiter update) { return waitersUpdater.getAndSet(future, update); @@ -1555,7 +1549,6 @@ boolean casListeners(AbstractFuture future, @CheckForNull Listener expect, Li } } - /** Performs a GAS operation on the {@link #listeners} field. */ @Override Listener gasListeners(AbstractFuture future, Listener update) { synchronized (future) { @@ -1567,7 +1560,6 @@ Listener gasListeners(AbstractFuture future, Listener update) { } } - /** Performs a GAS operation on the {@link #waiters} field. */ @Override Waiter gasWaiters(AbstractFuture future, Waiter update) { synchronized (future) { diff --git a/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java b/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java index 42cb8a1fbecc..f106dee2e074 100644 --- a/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java +++ b/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java @@ -85,7 +85,7 @@ public static TestSuite suite() { public void runTest() throws Exception { // First ensure that our classloaders are initializing the correct helper versions checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper"); - checkHelperVersion(NO_UNSAFE, "SafeAtomicHelper"); + checkHelperVersion(NO_UNSAFE, "AtomicReferenceFieldUpdaterAtomicHelper"); checkHelperVersion(NO_ATOMIC_REFERENCE_FIELD_UPDATER, "SynchronizedHelper"); // Run the corresponding AbstractFutureTest test method in a new classloader that disallows diff --git a/guava/src/com/google/common/util/concurrent/AbstractFuture.java b/guava/src/com/google/common/util/concurrent/AbstractFuture.java index d34b695342b2..d57d586fda7f 100644 --- a/guava/src/com/google/common/util/concurrent/AbstractFuture.java +++ b/guava/src/com/google/common/util/concurrent/AbstractFuture.java @@ -161,12 +161,13 @@ 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 'SafeAtomicHelper' - // The access control checks that ARFU does means the caller class has to be AbstractFuture - // instead of SafeAtomicHelper, so we annoyingly define these here + // 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 try { helper = - new SafeAtomicHelper( + new AtomicReferenceFieldUpdaterAtomicHelper( newUpdater(Waiter.class, Thread.class, "thread"), newUpdater(Waiter.class, Waiter.class, "next"), newUpdater(AbstractFuture.class, Waiter.class, "waiters"), @@ -196,7 +197,7 @@ public final boolean cancel(boolean mayInterruptIfRunning) { log.get() .log( Level.SEVERE, - "SafeAtomicHelper is broken!", + "AtomicReferenceFieldUpdaterAtomicHelper is broken!", thrownAtomicReferenceFieldUpdaterFailure); } } @@ -1325,21 +1326,21 @@ private abstract static class AtomicHelper { /** Non-volatile write of the waiter to the {@link Waiter#next} field. */ abstract void putNext(Waiter waiter, @CheckForNull Waiter newValue); - /** Performs a CAS operation on the {@link #waiters} field. */ + /** Performs a CAS operation on the {@link AbstractFuture#waiters} field. */ abstract boolean casWaiters( AbstractFuture future, @CheckForNull Waiter expect, @CheckForNull Waiter update); - /** Performs a CAS operation on the {@link #listeners} field. */ + /** Performs a CAS operation on the {@link AbstractFuture#listeners} field. */ abstract boolean casListeners( AbstractFuture future, @CheckForNull Listener expect, Listener update); - /** Performs a GAS operation on the {@link #waiters} field. */ + /** Performs a GAS operation on the {@link AbstractFuture#waiters} field. */ abstract Waiter gasWaiters(AbstractFuture future, Waiter update); - /** Performs a GAS operation on the {@link #listeners} field. */ + /** Performs a GAS operation on the {@link AbstractFuture#listeners} field. */ abstract Listener gasListeners(AbstractFuture future, Listener update); - /** Performs a CAS operation on the {@link #value} field. */ + /** Performs a CAS operation on the {@link AbstractFuture#value} field. */ abstract boolean casValue(AbstractFuture future, @CheckForNull Object expect, Object update); } @@ -1407,32 +1408,27 @@ void putNext(Waiter waiter, @CheckForNull Waiter newValue) { UNSAFE.putObject(waiter, WAITER_NEXT_OFFSET, newValue); } - /** Performs a CAS operation on the {@link #waiters} field. */ @Override boolean casWaiters( AbstractFuture future, @CheckForNull Waiter expect, @CheckForNull Waiter update) { return UNSAFE.compareAndSwapObject(future, WAITERS_OFFSET, expect, update); } - /** Performs a CAS operation on the {@link #listeners} field. */ @Override boolean casListeners(AbstractFuture future, @CheckForNull Listener expect, Listener update) { return UNSAFE.compareAndSwapObject(future, LISTENERS_OFFSET, expect, update); } - /** Performs a GAS operation on the {@link #listeners} field. */ @Override Listener gasListeners(AbstractFuture future, Listener update) { return (Listener) UNSAFE.getAndSetObject(future, LISTENERS_OFFSET, update); } - /** Performs a GAS operation on the {@link #waiters} field. */ @Override Waiter gasWaiters(AbstractFuture future, Waiter update) { return (Waiter) UNSAFE.getAndSetObject(future, WAITERS_OFFSET, update); } - /** Performs a CAS operation on the {@link #value} field. */ @Override boolean casValue(AbstractFuture future, @CheckForNull Object expect, Object update) { return UNSAFE.compareAndSwapObject(future, VALUE_OFFSET, expect, update); @@ -1440,14 +1436,14 @@ boolean casValue(AbstractFuture future, @CheckForNull Object expect, Object u } /** {@link AtomicHelper} based on {@link AtomicReferenceFieldUpdater}. */ - private static final class SafeAtomicHelper extends AtomicHelper { + 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; - SafeAtomicHelper( + AtomicReferenceFieldUpdaterAtomicHelper( AtomicReferenceFieldUpdater waiterThreadUpdater, AtomicReferenceFieldUpdater waiterNextUpdater, AtomicReferenceFieldUpdater, Waiter> waitersUpdater, @@ -1481,13 +1477,11 @@ boolean casListeners(AbstractFuture future, @CheckForNull Listener expect, Li return listenersUpdater.compareAndSet(future, expect, update); } - /** Performs a GAS operation on the {@link #listeners} field. */ @Override Listener gasListeners(AbstractFuture future, Listener update) { return listenersUpdater.getAndSet(future, update); } - /** Performs a GAS operation on the {@link #waiters} field. */ @Override Waiter gasWaiters(AbstractFuture future, Waiter update) { return waitersUpdater.getAndSet(future, update); @@ -1539,7 +1533,6 @@ boolean casListeners(AbstractFuture future, @CheckForNull Listener expect, Li } } - /** Performs a GAS operation on the {@link #listeners} field. */ @Override Listener gasListeners(AbstractFuture future, Listener update) { synchronized (future) { @@ -1551,7 +1544,6 @@ Listener gasListeners(AbstractFuture future, Listener update) { } } - /** Performs a GAS operation on the {@link #waiters} field. */ @Override Waiter gasWaiters(AbstractFuture future, Waiter update) { synchronized (future) {