Skip to content

Commit 75da924

Browse files
cpovirkGoogle Java Core Libraries
authored andcommitted
Use AtomicReferenceFieldUpdater instead of VarHandle when guava-android runs under a JVM.
For much more discussion, see the code comments, especially in the backport copy of `AbstractFutureState`. (For a bit more on performance, see https://shipilev.net/blog/2015/faster-atomic-fu/, including its notes that `Unsafe` is not necessarily faster than `AtomicReferenceFieldUpdater`.) Now that we no longer use `VarHandle` under Android, we can remove some Proguard rules for it: - We no longer need the `-dontwarn` rule for `VarHandleAtomicHelper`, since that type no longer exists in `guava-android`. - We no longer need the `-assumevalues` rule for `mightBeAndroid`, since it served only to strip the `VarHandle` code (to hide it from optimizers that run on the optimized code). And all else being equal, we'd rather _not_ have that rule _just in case_ someone is running `guava-android` through an optimizer and using it under the JVM (in which case we'd like to follow the JVM code path so that we don't try to use `Unsafe`). (OK, maybe it would be nice to keep the rule just so that Android doesn't have to perform a check of the `java.runtime.name` system property once during `AbstractFutureState` initialization. If anyone finds this to be an issue, please let us know.) I've also updated the tests to better reflect which ones we run only under a JVM, not in an Android emulator. (I should really have done that back in cl/742859752.) Fixes #7769 RELNOTES=`util.concurrent`: Removed our `VarHandle` code from `guava-android`. While the code was never used at runtime under Android, it was causing [problems under the Android Gradle Plugin](#7769) with a `minSdkVersion` below 26. To continue to avoid `sun.misc.Unsafe` under the JVM, `guava-android` will now always use `AtomicReferenceFieldUpdater` when run there. PiperOrigin-RevId: 746800729
1 parent 97c96e0 commit 75da924

File tree

8 files changed

+105
-158
lines changed

8 files changed

+105
-158
lines changed

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,11 @@
3030
@NullUnmarked
3131
public class AbstractFutureDefaultAtomicHelperTest extends TestCase {
3232
public void testUsingExpectedAtomicHelper() throws Exception {
33-
if (isJava8() || isAndroid()) {
33+
if (isAndroid()) {
3434
assertThat(AbstractFutureState.atomicHelperTypeForTest()).isEqualTo("UnsafeAtomicHelper");
3535
} else {
36-
assertThat(AbstractFutureState.atomicHelperTypeForTest()).isEqualTo("VarHandleAtomicHelper");
36+
assertThat(AbstractFutureState.atomicHelperTypeForTest())
37+
.isEqualTo("AtomicReferenceFieldUpdaterAtomicHelper");
3738
}
3839
}
3940

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

Lines changed: 34 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -89,26 +89,46 @@ public static TestSuite suite() {
8989

9090
@Override
9191
public void runTest() throws Exception {
92-
// First ensure that our classloaders are initializing the correct helper versions
93-
if (isJava8() || isAndroid()) {
94-
checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper");
95-
} else {
96-
checkHelperVersion(getClass().getClassLoader(), "VarHandleAtomicHelper");
97-
}
98-
checkHelperVersion(NO_VAR_HANDLE, "UnsafeAtomicHelper");
92+
/*
93+
* Note that we do not run this test under Android at the moment. For Android testing, see
94+
* AbstractFutureDefaultAtomicHelperTest.
95+
*/
96+
97+
// First, ensure that our classloaders are initializing the correct helper versions:
98+
99+
checkHelperVersion(getClass().getClassLoader(), "AtomicReferenceFieldUpdaterAtomicHelper");
100+
/*
101+
* Since we use AtomicReferenceFieldUpdaterAtomicHelper by default, we'll "obviously" use it
102+
* even when Unsafe isn't available. But it's nice to have a check here to make sure that
103+
* nothing somehow goes wrong as the JDK restricts access to Unsafe.
104+
*/
99105
checkHelperVersion(NO_UNSAFE, "AtomicReferenceFieldUpdaterAtomicHelper");
106+
/*
107+
* SynchronizedHelper is meant for Android, but our best way to test it is under the JVM.
108+
*
109+
* SynchronizedHelper also ends up getting used under the JVM during
110+
* AggregateFutureStateFallbackAtomicHelperTest, as discussed in a comment in
111+
* AggregateFutureState.
112+
*
113+
* Here, we check that we're able to force AbstractFutureState to select SynchronizedHelper, and
114+
* below, we actually run the AbstractFutureTest methods under that scenario.
115+
*/
100116
checkHelperVersion(NO_ATOMIC_REFERENCE_FIELD_UPDATER, "SynchronizedHelper");
101117

118+
// Then, run the actual tests under each alternative classloader:
119+
102120
/*
103-
* Under Java 8 or Android, there is no need to test the no-VarHandle case here: It's already
104-
* tested by the main AbstractFutureTest, which uses the default AtomicHelper, which we verified
105-
* above to be UnsafeAtomicHelper.
121+
* We don't need to test further under NO_UNSAFE: We verified that it selects
122+
* AtomicReferenceFieldUpdaterAtomicHelper, which is the default, which means that it's used
123+
* when we run AbstractFutureTest itself.
106124
*/
107-
if (!isJava8() && !isAndroid()) {
108-
runTestMethod(NO_VAR_HANDLE);
109-
}
110125

111-
runTestMethod(NO_UNSAFE);
126+
/*
127+
* We don't test UnsafeAtomicHelper here, since guava-android doesn't provide a way to use it
128+
* under the JVM. (We could arrange for one if we really wanted, but that will break once the
129+
* JDK further restricts access to Unsafe.) But we have coverage under an Android emulator,
130+
* which uses UnsafeAtomicHelper when it runs AbstractFutureTest itself.
131+
*/
112132

113133
runTestMethod(NO_ATOMIC_REFERENCE_FIELD_UPDATER);
114134
// TODO(lukes): assert that the logs are full of errors
@@ -164,8 +184,4 @@ public Class<?> loadClass(String name) throws ClassNotFoundException {
164184
private static boolean isJava8() {
165185
return JAVA_SPECIFICATION_VERSION.value().equals("1.8");
166186
}
167-
168-
private static boolean isAndroid() {
169-
return System.getProperty("java.runtime.name", "").contains("Android");
170-
}
171187
}

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,18 @@ public static TestSuite suite() {
8686

8787
@Override
8888
public void runTest() throws Exception {
89-
// First ensure that our classloaders are initializing the correct helper versions
89+
/*
90+
* Note that we do not run this test under Android at the moment. For Android testing, see
91+
* AggregateFutureStateDefaultAtomicHelperTest.
92+
*/
93+
94+
// First, ensure that our classloaders are initializing the correct helper versions:
95+
9096
checkHelperVersion(getClass().getClassLoader(), "SafeAtomicHelper");
9197
checkHelperVersion(NO_ATOMIC_FIELD_UPDATER, "SynchronizedAtomicHelper");
9298

99+
// Then, run the actual tests under each alternative classloader:
100+
93101
runTestMethod(NO_ATOMIC_FIELD_UPDATER);
94102
// TODO(lukes): assert that the logs are full of errors
95103
}

android/guava/src/com/google/common/util/concurrent/AbstractFutureState.java

Lines changed: 35 additions & 118 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,8 @@
2727
import com.google.common.annotations.VisibleForTesting;
2828
import com.google.common.util.concurrent.AbstractFuture.Listener;
2929
import com.google.common.util.concurrent.internal.InternalFutureFailureAccess;
30-
import com.google.j2objc.annotations.J2ObjCIncompatible;
3130
import com.google.j2objc.annotations.ReflectionSupport;
3231
import com.google.j2objc.annotations.RetainedLocalRef;
33-
import java.lang.invoke.MethodHandles;
34-
import java.lang.invoke.VarHandle;
3532
import java.lang.reflect.Field;
3633
import java.security.PrivilegedActionException;
3734
import java.security.PrivilegedExceptionAction;
@@ -348,8 +345,7 @@ void unpark() {
348345
Throwable thrownUnsafeFailure = null;
349346
Throwable thrownAtomicReferenceFieldUpdaterFailure = null;
350347

351-
helper = VarHandleAtomicHelperMaker.INSTANCE.tryMakeVarHandleAtomicHelper();
352-
if (helper == null) {
348+
if (mightBeAndroid()) {
353349
try {
354350
helper = new UnsafeAtomicHelper();
355351
} catch (Exception | Error unsafeFailure) { // sneaky checked exception
@@ -367,6 +363,39 @@ void unpark() {
367363
helper = new SynchronizedHelper();
368364
}
369365
}
366+
} else {
367+
/*
368+
* We avoid Unsafe, since newer JVMs produce warnings or even errors for attempts to use it.
369+
*
370+
* In guava-jre, we avoid Unsafe by using VarHandle instead. But if we have references to
371+
* VarHandle in guava-android, even if they're unused under Android, we cause errors under
372+
* AGP: https://github.com/google/guava/issues/7769.
373+
*
374+
* My impression is that an AtomicReferenceFieldUpdater in a static field is similarly fast to
375+
* Unsafe on modern JVMs (if perhaps not quite as fast as VarHandle?). However, I'm not sure
376+
* exactly what we've benchmarked, and we certainly haven't benchmarked as far back as JDK 8.
377+
* (We also haven't benchmarked under Android. We continue to use UnsafeAtomicHelper there so
378+
* that we don't change the performance there, for better or for worse.) Fortunately, JVM
379+
* users will typically use guava-jre, not guava-android, and guava-jre uses the VarHandle
380+
* implementation when possible.
381+
*/
382+
try {
383+
helper = new AtomicReferenceFieldUpdaterAtomicHelper();
384+
} catch (NoClassDefFoundError fromAggregateFutureStateFallbackAtomicHelperTest) {
385+
/*
386+
* AtomicReferenceFieldUpdaterAtomicHelper should always work on the JVM. (I mean, it
387+
* "should" always work on Android, too, but we know of a Samsung bug there :)) However, in
388+
* AggregateFutureStateFallbackAtomicHelperTest, we test what happens to AggregateFuture in
389+
* the case of the Samsung bug, and we do that by breaking AtomicReferenceFieldUpdater.
390+
* Breaking AtomicReferenceFieldUpdater not only forces AggregateFutureState to fall back to
391+
* another implementation but also forces AbstractFutureState to be able to do the
392+
* same—hence the try-catch here.
393+
*
394+
* (Really, we're fortunate that breaking AtomicReferenceFieldUpdater doesn't break _even
395+
* more_ things.)
396+
*/
397+
helper = new SynchronizedHelper();
398+
}
370399
}
371400
ATOMIC_HELPER = helper;
372401

@@ -403,7 +432,7 @@ void unpark() {
403432
* lookup would fail with an IllegalAccessException. That may then trigger use of Unsafe (possibly
404433
* with a warning under recent JVMs), or it may fall back even further to
405434
* AtomicReferenceFieldUpdaterAtomicHelper, which would fail with a similar problem to
406-
* VarHandleAtomicHelperMaker, forcing us all the way to SynchronizedAtomicHelper.
435+
* VarHandleAtomicHelperMaker, forcing us all the way to SynchronizedHelper.
407436
*
408437
* Additionally, it seems that nestmates do not help with runtime reflection under *Android*, even
409438
* when we use a newer -source and -target. That doesn't normally matter for AbstractFutureState,
@@ -512,43 +541,6 @@ static String atomicHelperTypeForTest() {
512541
return ATOMIC_HELPER.atomicHelperTypeForTest();
513542
}
514543

515-
private enum VarHandleAtomicHelperMaker {
516-
INSTANCE {
517-
/**
518-
* Implementation used by non-J2ObjC environments (aside, of course, from those that have
519-
* supersource for the entirety of {@link AbstractFutureState}).
520-
*/
521-
@Override
522-
@J2ObjCIncompatible
523-
@Nullable AtomicHelper tryMakeVarHandleAtomicHelper() {
524-
if (mightBeAndroid()) {
525-
return null;
526-
}
527-
try {
528-
/*
529-
* We first use reflection to check whether VarHandle exists. If we instead just tried to
530-
* load our class directly (which would trigger non-reflective loading of VarHandle) from
531-
* within a `try` block, then an error might be thrown even before we enter the `try`
532-
* block: https://github.com/google/truth/issues/333#issuecomment-765652454
533-
*
534-
* Also, it's nice that this approach should let us catch *only* ClassNotFoundException
535-
* instead of having to catch more broadly (potentially even including, say, a
536-
* StackOverflowError).
537-
*/
538-
Class.forName("java.lang.invoke.VarHandle");
539-
} catch (ClassNotFoundException beforeJava9) {
540-
return null;
541-
}
542-
return new VarHandleAtomicHelper();
543-
}
544-
};
545-
546-
/** Implementation used by J2ObjC environments, overridden for other environments. */
547-
@Nullable AtomicHelper tryMakeVarHandleAtomicHelper() {
548-
return null;
549-
}
550-
}
551-
552544
private abstract static class AtomicHelper {
553545
/** Non-volatile write of the thread to the {@link Waiter#thread} field. */
554546
abstract void putThread(Waiter waiter, Thread newValue);
@@ -577,81 +569,6 @@ abstract boolean casValue(
577569
abstract String atomicHelperTypeForTest();
578570
}
579571

580-
/** {@link AtomicHelper} based on {@link VarHandle}. */
581-
@J2ObjCIncompatible
582-
// We use this class only after confirming that VarHandle is available at runtime.
583-
@SuppressWarnings({"Java8ApiChecker", "Java7ApiChecker", "AndroidJdkLibsChecker"})
584-
@IgnoreJRERequirement
585-
private static final class VarHandleAtomicHelper extends AtomicHelper {
586-
static final VarHandle waiterThreadUpdater;
587-
static final VarHandle waiterNextUpdater;
588-
static final VarHandle waitersUpdater;
589-
static final VarHandle listenersUpdater;
590-
static final VarHandle valueUpdater;
591-
592-
static {
593-
MethodHandles.Lookup lookup = MethodHandles.lookup();
594-
try {
595-
waiterThreadUpdater = lookup.findVarHandle(Waiter.class, "thread", Thread.class);
596-
waiterNextUpdater = lookup.findVarHandle(Waiter.class, "next", Waiter.class);
597-
waitersUpdater =
598-
lookup.findVarHandle(AbstractFutureState.class, "waitersField", Waiter.class);
599-
listenersUpdater =
600-
lookup.findVarHandle(AbstractFutureState.class, "listenersField", Listener.class);
601-
valueUpdater = lookup.findVarHandle(AbstractFutureState.class, "valueField", Object.class);
602-
} catch (ReflectiveOperationException e) {
603-
// Those fields exist.
604-
throw newLinkageError(e);
605-
}
606-
}
607-
608-
@Override
609-
void putThread(Waiter waiter, Thread newValue) {
610-
waiterThreadUpdater.setRelease(waiter, newValue);
611-
}
612-
613-
@Override
614-
void putNext(Waiter waiter, @Nullable Waiter newValue) {
615-
waiterNextUpdater.setRelease(waiter, newValue);
616-
}
617-
618-
@Override
619-
boolean casWaiters(
620-
AbstractFutureState<?> future, @Nullable Waiter expect, @Nullable Waiter update) {
621-
return waitersUpdater.compareAndSet(future, expect, update);
622-
}
623-
624-
@Override
625-
boolean casListeners(
626-
AbstractFutureState<?> future, @Nullable Listener expect, Listener update) {
627-
return listenersUpdater.compareAndSet(future, expect, update);
628-
}
629-
630-
@Override
631-
@Nullable Listener gasListeners(AbstractFutureState<?> future, Listener update) {
632-
return (Listener) listenersUpdater.getAndSet(future, update);
633-
}
634-
635-
@Override
636-
@Nullable Waiter gasWaiters(AbstractFutureState<?> future, Waiter update) {
637-
return (Waiter) waitersUpdater.getAndSet(future, update);
638-
}
639-
640-
@Override
641-
boolean casValue(AbstractFutureState<?> future, @Nullable Object expect, Object update) {
642-
return valueUpdater.compareAndSet(future, expect, update);
643-
}
644-
645-
private static LinkageError newLinkageError(Throwable cause) {
646-
return new LinkageError(cause.toString(), cause);
647-
}
648-
649-
@Override
650-
String atomicHelperTypeForTest() {
651-
return "VarHandleAtomicHelper";
652-
}
653-
}
654-
655572
/**
656573
* {@link AtomicHelper} based on {@link sun.misc.Unsafe}.
657574
*

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

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,14 @@ public static TestSuite suite() {
8989

9090
@Override
9191
public void runTest() throws Exception {
92-
// First ensure that our classloaders are initializing the correct helper versions
93-
if (isJava8() || isAndroid()) {
92+
/*
93+
* Note that we do not run this test under Android at the moment. For Android testing, see
94+
* AbstractFutureDefaultAtomicHelperTest.
95+
*/
96+
97+
// First, ensure that our classloaders are initializing the correct helper versions:
98+
99+
if (isJava8()) {
94100
checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper");
95101
} else {
96102
checkHelperVersion(getClass().getClassLoader(), "VarHandleAtomicHelper");
@@ -99,12 +105,14 @@ public void runTest() throws Exception {
99105
checkHelperVersion(NO_UNSAFE, "AtomicReferenceFieldUpdaterAtomicHelper");
100106
checkHelperVersion(NO_ATOMIC_REFERENCE_FIELD_UPDATER, "SynchronizedHelper");
101107

108+
// Then, run the actual tests under each alternative classloader:
109+
102110
/*
103-
* Under Java 8 or Android, there is no need to test the no-VarHandle case here: It's already
104-
* tested by the main AbstractFutureTest, which uses the default AtomicHelper, which we verified
105-
* above to be UnsafeAtomicHelper.
111+
* Under Java 8, there is no need to test the no-VarHandle case here: It's already tested by the
112+
* main AbstractFutureTest, which uses the default AtomicHelper, which we verified above to be
113+
* UnsafeAtomicHelper.
106114
*/
107-
if (!isJava8() && !isAndroid()) {
115+
if (!isJava8()) {
108116
runTestMethod(NO_VAR_HANDLE);
109117
}
110118

@@ -164,8 +172,4 @@ public Class<?> loadClass(String name) throws ClassNotFoundException {
164172
private static boolean isJava8() {
165173
return JAVA_SPECIFICATION_VERSION.value().equals("1.8");
166174
}
167-
168-
private static boolean isAndroid() {
169-
return System.getProperty("java.runtime.name", "").contains("Android");
170-
}
171175
}

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,18 @@ public static TestSuite suite() {
8686

8787
@Override
8888
public void runTest() throws Exception {
89-
// First ensure that our classloaders are initializing the correct helper versions
89+
/*
90+
* Note that we do not run this test under Android at the moment. For Android testing, see
91+
* AggregateFutureStateDefaultAtomicHelperTest.
92+
*/
93+
94+
// First, ensure that our classloaders are initializing the correct helper versions:
95+
9096
checkHelperVersion(getClass().getClassLoader(), "SafeAtomicHelper");
9197
checkHelperVersion(NO_ATOMIC_FIELD_UPDATER, "SynchronizedAtomicHelper");
9298

99+
// Then, run the actual tests under each alternative classloader:
100+
93101
runTestMethod(NO_ATOMIC_FIELD_UPDATER);
94102
// TODO(lukes): assert that the logs are full of errors
95103
}

guava/src/com/google/common/util/concurrent/AbstractFutureState.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -403,7 +403,7 @@ void unpark() {
403403
* lookup would fail with an IllegalAccessException. That may then trigger use of Unsafe (possibly
404404
* with a warning under recent JVMs), or it may fall back even further to
405405
* AtomicReferenceFieldUpdaterAtomicHelper, which would fail with a similar problem to
406-
* VarHandleAtomicHelperMaker, forcing us all the way to SynchronizedAtomicHelper.
406+
* VarHandleAtomicHelperMaker, forcing us all the way to SynchronizedHelper.
407407
*
408408
* Additionally, it seems that nestmates do not help with runtime reflection under *Android*, even
409409
* when we use a newer -source and -target. That doesn't normally matter for AbstractFutureState,

proguard/concurrent.pro

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -47,10 +47,3 @@
4747
-keep class com.google.apphosting.api.ApiProxy {
4848
static *** getCurrentEnvironment (...);
4949
}
50-
51-
# b/407533570
52-
-dontwarn com.google.common.util.concurrent.AbstractFutureState$VarHandleAtomicHelper
53-
# See post-submit commentary on cl/743198569 about b/408047495.
54-
-assumevalues class com.google.common.util.concurrent.AbstractFutureState {
55-
boolean mightBeAndroid() return true;
56-
}

0 commit comments

Comments
 (0)