Skip to content

Commit

Permalink
Prefactoring for future changes that will avoid using Unsafe.
Browse files Browse the repository at this point in the history 
See #6806 (comment).

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
  • Loading branch information
@cpovirk
cpovirk authored and Google Java Core Libraries committed Dec 13, 2024
1 parent a124c1e commit 7283b8b
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 44 deletions.
2 changes: 1 addition & 1 deletion ...-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
34 changes: 13 additions & 21 deletions android/guava/src/com/google/common/util/concurrent/AbstractFuture.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down Expand Up @@ -196,7 +197,7 @@ public final boolean cancel(boolean mayInterruptIfRunning) {
log.get()
.log(
Level.SEVERE,
"SafeAtomicHelper is broken!",
"AtomicReferenceFieldUpdaterAtomicHelper is broken!",
thrownAtomicReferenceFieldUpdaterFailure);
}
}
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -1448,22 +1445,21 @@ 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);
}
}

/** {@link AtomicHelper} based on {@link AtomicReferenceFieldUpdater}. */
private static final class SafeAtomicHelper extends AtomicHelper {
private static final class AtomicReferenceFieldUpdaterAtomicHelper extends AtomicHelper {
final AtomicReferenceFieldUpdater<Waiter, Thread> waiterThreadUpdater;
final AtomicReferenceFieldUpdater<Waiter, Waiter> waiterNextUpdater;
final AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Waiter> waitersUpdater;
final AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Listener> listenersUpdater;
final AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Object> valueUpdater;

SafeAtomicHelper(
AtomicReferenceFieldUpdaterAtomicHelper(
AtomicReferenceFieldUpdater<Waiter, Thread> waiterThreadUpdater,
AtomicReferenceFieldUpdater<Waiter, Waiter> waiterNextUpdater,
AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Waiter> waitersUpdater,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion ...-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
34 changes: 13 additions & 21 deletions guava/src/com/google/common/util/concurrent/AbstractFuture.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down Expand Up @@ -196,7 +197,7 @@ public final boolean cancel(boolean mayInterruptIfRunning) {
log.get()
.log(
Level.SEVERE,
"SafeAtomicHelper is broken!",
"AtomicReferenceFieldUpdaterAtomicHelper is broken!",
thrownAtomicReferenceFieldUpdaterFailure);
}
}
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -1407,47 +1408,42 @@ 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);
}
}

/** {@link AtomicHelper} based on {@link AtomicReferenceFieldUpdater}. */
private static final class SafeAtomicHelper extends AtomicHelper {
private static final class AtomicReferenceFieldUpdaterAtomicHelper extends AtomicHelper {
final AtomicReferenceFieldUpdater<Waiter, Thread> waiterThreadUpdater;
final AtomicReferenceFieldUpdater<Waiter, Waiter> waiterNextUpdater;
final AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Waiter> waitersUpdater;
final AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Listener> listenersUpdater;
final AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Object> valueUpdater;

SafeAtomicHelper(
AtomicReferenceFieldUpdaterAtomicHelper(
AtomicReferenceFieldUpdater<Waiter, Thread> waiterThreadUpdater,
AtomicReferenceFieldUpdater<Waiter, Waiter> waiterNextUpdater,
AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Waiter> waitersUpdater,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down

0 comments on commit 7283b8b

Please sign in to comment.