Skip to content

Commit 660e5dc

Browse files
cpovirkGoogle Java Core Libraries
cpovirk
authored and
Google Java Core Libraries
committed
Prefactoring for future changes that will avoid using Unsafe.
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: 705868797
1 parent a124c1e commit 660e5dc

File tree

4 files changed

+28
-44
lines changed

4 files changed

+28
-44
lines changed

‎android/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ public static TestSuite suite() {
8585
public void runTest() throws Exception {
8686
// First ensure that our classloaders are initializing the correct helper versions
8787
checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper");
88-
checkHelperVersion(NO_UNSAFE, "SafeAtomicHelper");
88+
checkHelperVersion(NO_UNSAFE, "AtomicReferenceFieldUpdaterAtomicHelper");
8989
checkHelperVersion(NO_ATOMIC_REFERENCE_FIELD_UPDATER, "SynchronizedHelper");
9090

9191
// Run the corresponding AbstractFutureTest test method in a new classloader that disallows

‎android/guava/src/com/google/common/util/concurrent/AbstractFuture.java‎

Lines changed: 13 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -161,12 +161,13 @@ public final boolean cancel(boolean mayInterruptIfRunning) {
161161
helper = new UnsafeAtomicHelper();
162162
} catch (Exception | Error unsafeFailure) { // sneaky checked exception
163163
thrownUnsafeFailure = unsafeFailure;
164-
// catch absolutely everything and fall through to our 'SafeAtomicHelper'
165-
// The access control checks that ARFU does means the caller class has to be AbstractFuture
166-
// instead of SafeAtomicHelper, so we annoyingly define these here
164+
// catch absolutely everything and fall through to our
165+
// 'AtomicReferenceFieldUpdaterAtomicHelper' The access control checks that ARFU does means
166+
// the caller class has to be AbstractFuture instead of
167+
// AtomicReferenceFieldUpdaterAtomicHelper, so we annoyingly define these here
167168
try {
168169
helper =
169-
new SafeAtomicHelper(
170+
new AtomicReferenceFieldUpdaterAtomicHelper(
170171
newUpdater(Waiter.class, Thread.class, "thread"),
171172
newUpdater(Waiter.class, Waiter.class, "next"),
172173
newUpdater(AbstractFuture.class, Waiter.class, "waiters"),
@@ -196,7 +197,7 @@ public final boolean cancel(boolean mayInterruptIfRunning) {
196197
log.get()
197198
.log(
198199
Level.SEVERE,
199-
"SafeAtomicHelper is broken!",
200+
"AtomicReferenceFieldUpdaterAtomicHelper is broken!",
200201
thrownAtomicReferenceFieldUpdaterFailure);
201202
}
202203
}
@@ -1325,21 +1326,21 @@ private abstract static class AtomicHelper {
13251326
/** Non-volatile write of the waiter to the {@link Waiter#next} field. */
13261327
abstract void putNext(Waiter waiter, @CheckForNull Waiter newValue);
13271328

1328-
/** Performs a CAS operation on the {@link #waiters} field. */
1329+
/** Performs a CAS operation on the {@link AbstractFuture#waiters} field. */
13291330
abstract boolean casWaiters(
13301331
AbstractFuture<?> future, @CheckForNull Waiter expect, @CheckForNull Waiter update);
13311332

1332-
/** Performs a CAS operation on the {@link #listeners} field. */
1333+
/** Performs a CAS operation on the {@link AbstractFuture#listeners} field. */
13331334
abstract boolean casListeners(
13341335
AbstractFuture<?> future, @CheckForNull Listener expect, Listener update);
13351336

1336-
/** Performs a GAS operation on the {@link #waiters} field. */
1337+
/** Performs a GAS operation on the {@link AbstractFuture#waiters} field. */
13371338
abstract Waiter gasWaiters(AbstractFuture<?> future, Waiter update);
13381339

1339-
/** Performs a GAS operation on the {@link #listeners} field. */
1340+
/** Performs a GAS operation on the {@link AbstractFuture#listeners} field. */
13401341
abstract Listener gasListeners(AbstractFuture<?> future, Listener update);
13411342

1342-
/** Performs a CAS operation on the {@link #value} field. */
1343+
/** Performs a CAS operation on the {@link AbstractFuture#value} field. */
13431344
abstract boolean casValue(AbstractFuture<?> future, @CheckForNull Object expect, Object update);
13441345
}
13451346

@@ -1407,20 +1408,17 @@ void putNext(Waiter waiter, @CheckForNull Waiter newValue) {
14071408
UNSAFE.putObject(waiter, WAITER_NEXT_OFFSET, newValue);
14081409
}
14091410

1410-
/** Performs a CAS operation on the {@link #waiters} field. */
14111411
@Override
14121412
boolean casWaiters(
14131413
AbstractFuture<?> future, @CheckForNull Waiter expect, @CheckForNull Waiter update) {
14141414
return UNSAFE.compareAndSwapObject(future, WAITERS_OFFSET, expect, update);
14151415
}
14161416

1417-
/** Performs a CAS operation on the {@link #listeners} field. */
14181417
@Override
14191418
boolean casListeners(AbstractFuture<?> future, @CheckForNull Listener expect, Listener update) {
14201419
return UNSAFE.compareAndSwapObject(future, LISTENERS_OFFSET, expect, update);
14211420
}
14221421

1423-
/** Performs a GAS operation on the {@link #listeners} field. */
14241422
@Override
14251423
Listener gasListeners(AbstractFuture<?> future, Listener update) {
14261424
while (true) {
@@ -1434,7 +1432,6 @@ Listener gasListeners(AbstractFuture<?> future, Listener update) {
14341432
}
14351433
}
14361434

1437-
/** Performs a GAS operation on the {@link #waiters} field. */
14381435
@Override
14391436
Waiter gasWaiters(AbstractFuture<?> future, Waiter update) {
14401437
while (true) {
@@ -1448,22 +1445,21 @@ Waiter gasWaiters(AbstractFuture<?> future, Waiter update) {
14481445
}
14491446
}
14501447

1451-
/** Performs a CAS operation on the {@link #value} field. */
14521448
@Override
14531449
boolean casValue(AbstractFuture<?> future, @CheckForNull Object expect, Object update) {
14541450
return UNSAFE.compareAndSwapObject(future, VALUE_OFFSET, expect, update);
14551451
}
14561452
}
14571453

14581454
/** {@link AtomicHelper} based on {@link AtomicReferenceFieldUpdater}. */
1459-
private static final class SafeAtomicHelper extends AtomicHelper {
1455+
private static final class AtomicReferenceFieldUpdaterAtomicHelper extends AtomicHelper {
14601456
final AtomicReferenceFieldUpdater<Waiter, Thread> waiterThreadUpdater;
14611457
final AtomicReferenceFieldUpdater<Waiter, Waiter> waiterNextUpdater;
14621458
final AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Waiter> waitersUpdater;
14631459
final AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Listener> listenersUpdater;
14641460
final AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Object> valueUpdater;
14651461

1466-
SafeAtomicHelper(
1462+
AtomicReferenceFieldUpdaterAtomicHelper(
14671463
AtomicReferenceFieldUpdater<Waiter, Thread> waiterThreadUpdater,
14681464
AtomicReferenceFieldUpdater<Waiter, Waiter> waiterNextUpdater,
14691465
AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Waiter> waitersUpdater,
@@ -1497,13 +1493,11 @@ boolean casListeners(AbstractFuture<?> future, @CheckForNull Listener expect, Li
14971493
return listenersUpdater.compareAndSet(future, expect, update);
14981494
}
14991495

1500-
/** Performs a GAS operation on the {@link #listeners} field. */
15011496
@Override
15021497
Listener gasListeners(AbstractFuture<?> future, Listener update) {
15031498
return listenersUpdater.getAndSet(future, update);
15041499
}
15051500

1506-
/** Performs a GAS operation on the {@link #waiters} field. */
15071501
@Override
15081502
Waiter gasWaiters(AbstractFuture<?> future, Waiter update) {
15091503
return waitersUpdater.getAndSet(future, update);
@@ -1555,7 +1549,6 @@ boolean casListeners(AbstractFuture<?> future, @CheckForNull Listener expect, Li
15551549
}
15561550
}
15571551

1558-
/** Performs a GAS operation on the {@link #listeners} field. */
15591552
@Override
15601553
Listener gasListeners(AbstractFuture<?> future, Listener update) {
15611554
synchronized (future) {
@@ -1567,7 +1560,6 @@ Listener gasListeners(AbstractFuture<?> future, Listener update) {
15671560
}
15681561
}
15691562

1570-
/** Performs a GAS operation on the {@link #waiters} field. */
15711563
@Override
15721564
Waiter gasWaiters(AbstractFuture<?> future, Waiter update) {
15731565
synchronized (future) {

‎guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ public static TestSuite suite() {
8585
public void runTest() throws Exception {
8686
// First ensure that our classloaders are initializing the correct helper versions
8787
checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper");
88-
checkHelperVersion(NO_UNSAFE, "SafeAtomicHelper");
88+
checkHelperVersion(NO_UNSAFE, "AtomicReferenceFieldUpdaterAtomicHelper");
8989
checkHelperVersion(NO_ATOMIC_REFERENCE_FIELD_UPDATER, "SynchronizedHelper");
9090

9191
// Run the corresponding AbstractFutureTest test method in a new classloader that disallows

‎guava/src/com/google/common/util/concurrent/AbstractFuture.java‎

Lines changed: 13 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -161,12 +161,13 @@ public final boolean cancel(boolean mayInterruptIfRunning) {
161161
helper = new UnsafeAtomicHelper();
162162
} catch (Exception | Error unsafeFailure) { // sneaky checked exception
163163
thrownUnsafeFailure = unsafeFailure;
164-
// catch absolutely everything and fall through to our 'SafeAtomicHelper'
165-
// The access control checks that ARFU does means the caller class has to be AbstractFuture
166-
// instead of SafeAtomicHelper, so we annoyingly define these here
164+
// catch absolutely everything and fall through to our
165+
// 'AtomicReferenceFieldUpdaterAtomicHelper' The access control checks that ARFU does means
166+
// the caller class has to be AbstractFuture instead of
167+
// AtomicReferenceFieldUpdaterAtomicHelper, so we annoyingly define these here
167168
try {
168169
helper =
169-
new SafeAtomicHelper(
170+
new AtomicReferenceFieldUpdaterAtomicHelper(
170171
newUpdater(Waiter.class, Thread.class, "thread"),
171172
newUpdater(Waiter.class, Waiter.class, "next"),
172173
newUpdater(AbstractFuture.class, Waiter.class, "waiters"),
@@ -196,7 +197,7 @@ public final boolean cancel(boolean mayInterruptIfRunning) {
196197
log.get()
197198
.log(
198199
Level.SEVERE,
199-
"SafeAtomicHelper is broken!",
200+
"AtomicReferenceFieldUpdaterAtomicHelper is broken!",
200201
thrownAtomicReferenceFieldUpdaterFailure);
201202
}
202203
}
@@ -1325,21 +1326,21 @@ private abstract static class AtomicHelper {
13251326
/** Non-volatile write of the waiter to the {@link Waiter#next} field. */
13261327
abstract void putNext(Waiter waiter, @CheckForNull Waiter newValue);
13271328

1328-
/** Performs a CAS operation on the {@link #waiters} field. */
1329+
/** Performs a CAS operation on the {@link AbstractFuture#waiters} field. */
13291330
abstract boolean casWaiters(
13301331
AbstractFuture<?> future, @CheckForNull Waiter expect, @CheckForNull Waiter update);
13311332

1332-
/** Performs a CAS operation on the {@link #listeners} field. */
1333+
/** Performs a CAS operation on the {@link AbstractFuture#listeners} field. */
13331334
abstract boolean casListeners(
13341335
AbstractFuture<?> future, @CheckForNull Listener expect, Listener update);
13351336

1336-
/** Performs a GAS operation on the {@link #waiters} field. */
1337+
/** Performs a GAS operation on the {@link AbstractFuture#waiters} field. */
13371338
abstract Waiter gasWaiters(AbstractFuture<?> future, Waiter update);
13381339

1339-
/** Performs a GAS operation on the {@link #listeners} field. */
1340+
/** Performs a GAS operation on the {@link AbstractFuture#listeners} field. */
13401341
abstract Listener gasListeners(AbstractFuture<?> future, Listener update);
13411342

1342-
/** Performs a CAS operation on the {@link #value} field. */
1343+
/** Performs a CAS operation on the {@link AbstractFuture#value} field. */
13431344
abstract boolean casValue(AbstractFuture<?> future, @CheckForNull Object expect, Object update);
13441345
}
13451346

@@ -1407,47 +1408,42 @@ void putNext(Waiter waiter, @CheckForNull Waiter newValue) {
14071408
UNSAFE.putObject(waiter, WAITER_NEXT_OFFSET, newValue);
14081409
}
14091410

1410-
/** Performs a CAS operation on the {@link #waiters} field. */
14111411
@Override
14121412
boolean casWaiters(
14131413
AbstractFuture<?> future, @CheckForNull Waiter expect, @CheckForNull Waiter update) {
14141414
return UNSAFE.compareAndSwapObject(future, WAITERS_OFFSET, expect, update);
14151415
}
14161416

1417-
/** Performs a CAS operation on the {@link #listeners} field. */
14181417
@Override
14191418
boolean casListeners(AbstractFuture<?> future, @CheckForNull Listener expect, Listener update) {
14201419
return UNSAFE.compareAndSwapObject(future, LISTENERS_OFFSET, expect, update);
14211420
}
14221421

1423-
/** Performs a GAS operation on the {@link #listeners} field. */
14241422
@Override
14251423
Listener gasListeners(AbstractFuture<?> future, Listener update) {
14261424
return (Listener) UNSAFE.getAndSetObject(future, LISTENERS_OFFSET, update);
14271425
}
14281426

1429-
/** Performs a GAS operation on the {@link #waiters} field. */
14301427
@Override
14311428
Waiter gasWaiters(AbstractFuture<?> future, Waiter update) {
14321429
return (Waiter) UNSAFE.getAndSetObject(future, WAITERS_OFFSET, update);
14331430
}
14341431

1435-
/** Performs a CAS operation on the {@link #value} field. */
14361432
@Override
14371433
boolean casValue(AbstractFuture<?> future, @CheckForNull Object expect, Object update) {
14381434
return UNSAFE.compareAndSwapObject(future, VALUE_OFFSET, expect, update);
14391435
}
14401436
}
14411437

14421438
/** {@link AtomicHelper} based on {@link AtomicReferenceFieldUpdater}. */
1443-
private static final class SafeAtomicHelper extends AtomicHelper {
1439+
private static final class AtomicReferenceFieldUpdaterAtomicHelper extends AtomicHelper {
14441440
final AtomicReferenceFieldUpdater<Waiter, Thread> waiterThreadUpdater;
14451441
final AtomicReferenceFieldUpdater<Waiter, Waiter> waiterNextUpdater;
14461442
final AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Waiter> waitersUpdater;
14471443
final AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Listener> listenersUpdater;
14481444
final AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Object> valueUpdater;
14491445

1450-
SafeAtomicHelper(
1446+
AtomicReferenceFieldUpdaterAtomicHelper(
14511447
AtomicReferenceFieldUpdater<Waiter, Thread> waiterThreadUpdater,
14521448
AtomicReferenceFieldUpdater<Waiter, Waiter> waiterNextUpdater,
14531449
AtomicReferenceFieldUpdater<? super AbstractFuture<?>, Waiter> waitersUpdater,
@@ -1481,13 +1477,11 @@ boolean casListeners(AbstractFuture<?> future, @CheckForNull Listener expect, Li
14811477
return listenersUpdater.compareAndSet(future, expect, update);
14821478
}
14831479

1484-
/** Performs a GAS operation on the {@link #listeners} field. */
14851480
@Override
14861481
Listener gasListeners(AbstractFuture<?> future, Listener update) {
14871482
return listenersUpdater.getAndSet(future, update);
14881483
}
14891484

1490-
/** Performs a GAS operation on the {@link #waiters} field. */
14911485
@Override
14921486
Waiter gasWaiters(AbstractFuture<?> future, Waiter update) {
14931487
return waitersUpdater.getAndSet(future, update);
@@ -1539,7 +1533,6 @@ boolean casListeners(AbstractFuture<?> future, @CheckForNull Listener expect, Li
15391533
}
15401534
}
15411535

1542-
/** Performs a GAS operation on the {@link #listeners} field. */
15431536
@Override
15441537
Listener gasListeners(AbstractFuture<?> future, Listener update) {
15451538
synchronized (future) {
@@ -1551,7 +1544,6 @@ Listener gasListeners(AbstractFuture<?> future, Listener update) {
15511544
}
15521545
}
15531546

1554-
/** Performs a GAS operation on the {@link #waiters} field. */
15551547
@Override
15561548
Waiter gasWaiters(AbstractFuture<?> future, Waiter update) {
15571549
synchronized (future) {

0 commit comments

Comments
 (0)