From 75da92419a7d414bd3de23c89f2ddfd83754767d Mon Sep 17 00:00:00 2001 From: cpovirk Date: Sat, 12 Apr 2025 06:05:47 -0700 Subject: [PATCH] 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 https://github.com/google/guava/issues/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](https://github.com/google/guava/issues/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 --- ...AbstractFutureDefaultAtomicHelperTest.java | 5 +- ...bstractFutureFallbackAtomicHelperTest.java | 52 +++--- ...teFutureStateFallbackAtomicHelperTest.java | 10 +- .../util/concurrent/AbstractFutureState.java | 153 ++++-------------- ...bstractFutureFallbackAtomicHelperTest.java | 24 +-- ...teFutureStateFallbackAtomicHelperTest.java | 10 +- .../util/concurrent/AbstractFutureState.java | 2 +- proguard/concurrent.pro | 7 - 8 files changed, 105 insertions(+), 158 deletions(-) diff --git a/android/guava-tests/test/com/google/common/util/concurrent/AbstractFutureDefaultAtomicHelperTest.java b/android/guava-tests/test/com/google/common/util/concurrent/AbstractFutureDefaultAtomicHelperTest.java index 4088db1bee41..ac612b0fd34e 100644 --- a/android/guava-tests/test/com/google/common/util/concurrent/AbstractFutureDefaultAtomicHelperTest.java +++ b/android/guava-tests/test/com/google/common/util/concurrent/AbstractFutureDefaultAtomicHelperTest.java @@ -30,10 +30,11 @@ @NullUnmarked public class AbstractFutureDefaultAtomicHelperTest extends TestCase { public void testUsingExpectedAtomicHelper() throws Exception { - if (isJava8() || isAndroid()) { + if (isAndroid()) { assertThat(AbstractFutureState.atomicHelperTypeForTest()).isEqualTo("UnsafeAtomicHelper"); } else { - assertThat(AbstractFutureState.atomicHelperTypeForTest()).isEqualTo("VarHandleAtomicHelper"); + assertThat(AbstractFutureState.atomicHelperTypeForTest()) + .isEqualTo("AtomicReferenceFieldUpdaterAtomicHelper"); } } 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 b766e8659ede..e38be070cc1e 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 @@ -89,26 +89,46 @@ public static TestSuite suite() { @Override public void runTest() throws Exception { - // First ensure that our classloaders are initializing the correct helper versions - if (isJava8() || isAndroid()) { - checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper"); - } else { - checkHelperVersion(getClass().getClassLoader(), "VarHandleAtomicHelper"); - } - checkHelperVersion(NO_VAR_HANDLE, "UnsafeAtomicHelper"); + /* + * Note that we do not run this test under Android at the moment. For Android testing, see + * AbstractFutureDefaultAtomicHelperTest. + */ + + // First, ensure that our classloaders are initializing the correct helper versions: + + checkHelperVersion(getClass().getClassLoader(), "AtomicReferenceFieldUpdaterAtomicHelper"); + /* + * Since we use AtomicReferenceFieldUpdaterAtomicHelper by default, we'll "obviously" use it + * even when Unsafe isn't available. But it's nice to have a check here to make sure that + * nothing somehow goes wrong as the JDK restricts access to Unsafe. + */ checkHelperVersion(NO_UNSAFE, "AtomicReferenceFieldUpdaterAtomicHelper"); + /* + * SynchronizedHelper is meant for Android, but our best way to test it is under the JVM. + * + * SynchronizedHelper also ends up getting used under the JVM during + * AggregateFutureStateFallbackAtomicHelperTest, as discussed in a comment in + * AggregateFutureState. + * + * Here, we check that we're able to force AbstractFutureState to select SynchronizedHelper, and + * below, we actually run the AbstractFutureTest methods under that scenario. + */ checkHelperVersion(NO_ATOMIC_REFERENCE_FIELD_UPDATER, "SynchronizedHelper"); + // Then, run the actual tests under each alternative classloader: + /* - * Under Java 8 or Android, there is no need to test the no-VarHandle case here: It's already - * tested by the main AbstractFutureTest, which uses the default AtomicHelper, which we verified - * above to be UnsafeAtomicHelper. + * We don't need to test further under NO_UNSAFE: We verified that it selects + * AtomicReferenceFieldUpdaterAtomicHelper, which is the default, which means that it's used + * when we run AbstractFutureTest itself. */ - if (!isJava8() && !isAndroid()) { - runTestMethod(NO_VAR_HANDLE); - } - runTestMethod(NO_UNSAFE); + /* + * We don't test UnsafeAtomicHelper here, since guava-android doesn't provide a way to use it + * under the JVM. (We could arrange for one if we really wanted, but that will break once the + * JDK further restricts access to Unsafe.) But we have coverage under an Android emulator, + * which uses UnsafeAtomicHelper when it runs AbstractFutureTest itself. + */ runTestMethod(NO_ATOMIC_REFERENCE_FIELD_UPDATER); // TODO(lukes): assert that the logs are full of errors @@ -164,8 +184,4 @@ public Class loadClass(String name) throws ClassNotFoundException { private static boolean isJava8() { return JAVA_SPECIFICATION_VERSION.value().equals("1.8"); } - - private static boolean isAndroid() { - return System.getProperty("java.runtime.name", "").contains("Android"); - } } 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 e9d45d645f77..6c6729b2fcfd 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 @@ -86,10 +86,18 @@ public static TestSuite suite() { @Override public void runTest() throws Exception { - // First ensure that our classloaders are initializing the correct helper versions + /* + * Note that we do not run this test under Android at the moment. For Android testing, see + * AggregateFutureStateDefaultAtomicHelperTest. + */ + + // First, ensure that our classloaders are initializing the correct helper versions: + checkHelperVersion(getClass().getClassLoader(), "SafeAtomicHelper"); checkHelperVersion(NO_ATOMIC_FIELD_UPDATER, "SynchronizedAtomicHelper"); + // Then, run the actual tests under each alternative classloader: + runTestMethod(NO_ATOMIC_FIELD_UPDATER); // TODO(lukes): assert that the logs are full of errors } diff --git a/android/guava/src/com/google/common/util/concurrent/AbstractFutureState.java b/android/guava/src/com/google/common/util/concurrent/AbstractFutureState.java index 7f439e13eae1..bbf43f5b9e2e 100644 --- a/android/guava/src/com/google/common/util/concurrent/AbstractFutureState.java +++ b/android/guava/src/com/google/common/util/concurrent/AbstractFutureState.java @@ -27,11 +27,8 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.util.concurrent.AbstractFuture.Listener; import com.google.common.util.concurrent.internal.InternalFutureFailureAccess; -import com.google.j2objc.annotations.J2ObjCIncompatible; import com.google.j2objc.annotations.ReflectionSupport; import com.google.j2objc.annotations.RetainedLocalRef; -import java.lang.invoke.MethodHandles; -import java.lang.invoke.VarHandle; import java.lang.reflect.Field; import java.security.PrivilegedActionException; import java.security.PrivilegedExceptionAction; @@ -348,8 +345,7 @@ void unpark() { Throwable thrownUnsafeFailure = null; Throwable thrownAtomicReferenceFieldUpdaterFailure = null; - helper = VarHandleAtomicHelperMaker.INSTANCE.tryMakeVarHandleAtomicHelper(); - if (helper == null) { + if (mightBeAndroid()) { try { helper = new UnsafeAtomicHelper(); } catch (Exception | Error unsafeFailure) { // sneaky checked exception @@ -367,6 +363,39 @@ void unpark() { helper = new SynchronizedHelper(); } } + } else { + /* + * We avoid Unsafe, since newer JVMs produce warnings or even errors for attempts to use it. + * + * In guava-jre, we avoid Unsafe by using VarHandle instead. But if we have references to + * VarHandle in guava-android, even if they're unused under Android, we cause errors under + * AGP: https://github.com/google/guava/issues/7769. + * + * My impression is that an AtomicReferenceFieldUpdater in a static field is similarly fast to + * Unsafe on modern JVMs (if perhaps not quite as fast as VarHandle?). However, I'm not sure + * exactly what we've benchmarked, and we certainly haven't benchmarked as far back as JDK 8. + * (We also haven't benchmarked under Android. We continue to use UnsafeAtomicHelper there so + * that we don't change the performance there, for better or for worse.) Fortunately, JVM + * users will typically use guava-jre, not guava-android, and guava-jre uses the VarHandle + * implementation when possible. + */ + try { + helper = new AtomicReferenceFieldUpdaterAtomicHelper(); + } catch (NoClassDefFoundError fromAggregateFutureStateFallbackAtomicHelperTest) { + /* + * AtomicReferenceFieldUpdaterAtomicHelper should always work on the JVM. (I mean, it + * "should" always work on Android, too, but we know of a Samsung bug there :)) However, in + * AggregateFutureStateFallbackAtomicHelperTest, we test what happens to AggregateFuture in + * the case of the Samsung bug, and we do that by breaking AtomicReferenceFieldUpdater. + * Breaking AtomicReferenceFieldUpdater not only forces AggregateFutureState to fall back to + * another implementation but also forces AbstractFutureState to be able to do the + * same—hence the try-catch here. + * + * (Really, we're fortunate that breaking AtomicReferenceFieldUpdater doesn't break _even + * more_ things.) + */ + helper = new SynchronizedHelper(); + } } ATOMIC_HELPER = helper; @@ -403,7 +432,7 @@ void unpark() { * lookup would fail with an IllegalAccessException. That may then trigger use of Unsafe (possibly * with a warning under recent JVMs), or it may fall back even further to * AtomicReferenceFieldUpdaterAtomicHelper, which would fail with a similar problem to - * VarHandleAtomicHelperMaker, forcing us all the way to SynchronizedAtomicHelper. + * VarHandleAtomicHelperMaker, forcing us all the way to SynchronizedHelper. * * Additionally, it seems that nestmates do not help with runtime reflection under *Android*, even * when we use a newer -source and -target. That doesn't normally matter for AbstractFutureState, @@ -512,43 +541,6 @@ static String atomicHelperTypeForTest() { return ATOMIC_HELPER.atomicHelperTypeForTest(); } - private enum VarHandleAtomicHelperMaker { - INSTANCE { - /** - * Implementation used by non-J2ObjC environments (aside, of course, from those that have - * supersource for the entirety of {@link AbstractFutureState}). - */ - @Override - @J2ObjCIncompatible - @Nullable AtomicHelper tryMakeVarHandleAtomicHelper() { - if (mightBeAndroid()) { - return null; - } - try { - /* - * We first use reflection to check whether VarHandle exists. If we instead just tried to - * load our class directly (which would trigger non-reflective loading of VarHandle) from - * within a `try` block, then an error might be thrown even before we enter the `try` - * block: https://github.com/google/truth/issues/333#issuecomment-765652454 - * - * Also, it's nice that this approach should let us catch *only* ClassNotFoundException - * instead of having to catch more broadly (potentially even including, say, a - * StackOverflowError). - */ - Class.forName("java.lang.invoke.VarHandle"); - } catch (ClassNotFoundException beforeJava9) { - return null; - } - return new VarHandleAtomicHelper(); - } - }; - - /** Implementation used by J2ObjC environments, overridden for other environments. */ - @Nullable AtomicHelper tryMakeVarHandleAtomicHelper() { - return null; - } - } - private abstract static class AtomicHelper { /** Non-volatile write of the thread to the {@link Waiter#thread} field. */ abstract void putThread(Waiter waiter, Thread newValue); @@ -577,81 +569,6 @@ abstract boolean casValue( abstract String atomicHelperTypeForTest(); } - /** {@link AtomicHelper} based on {@link VarHandle}. */ - @J2ObjCIncompatible - // We use this class only after confirming that VarHandle is available at runtime. - @SuppressWarnings({"Java8ApiChecker", "Java7ApiChecker", "AndroidJdkLibsChecker"}) - @IgnoreJRERequirement - private static final class VarHandleAtomicHelper extends AtomicHelper { - static final VarHandle waiterThreadUpdater; - static final VarHandle waiterNextUpdater; - static final VarHandle waitersUpdater; - static final VarHandle listenersUpdater; - static final VarHandle valueUpdater; - - static { - MethodHandles.Lookup lookup = MethodHandles.lookup(); - try { - waiterThreadUpdater = lookup.findVarHandle(Waiter.class, "thread", Thread.class); - waiterNextUpdater = lookup.findVarHandle(Waiter.class, "next", Waiter.class); - waitersUpdater = - lookup.findVarHandle(AbstractFutureState.class, "waitersField", Waiter.class); - listenersUpdater = - lookup.findVarHandle(AbstractFutureState.class, "listenersField", Listener.class); - valueUpdater = lookup.findVarHandle(AbstractFutureState.class, "valueField", Object.class); - } catch (ReflectiveOperationException e) { - // Those fields exist. - throw newLinkageError(e); - } - } - - @Override - void putThread(Waiter waiter, Thread newValue) { - waiterThreadUpdater.setRelease(waiter, newValue); - } - - @Override - void putNext(Waiter waiter, @Nullable Waiter newValue) { - waiterNextUpdater.setRelease(waiter, newValue); - } - - @Override - boolean casWaiters( - AbstractFutureState future, @Nullable Waiter expect, @Nullable Waiter update) { - return waitersUpdater.compareAndSet(future, expect, update); - } - - @Override - boolean casListeners( - AbstractFutureState future, @Nullable Listener expect, Listener update) { - return listenersUpdater.compareAndSet(future, expect, update); - } - - @Override - @Nullable Listener gasListeners(AbstractFutureState future, Listener update) { - return (Listener) listenersUpdater.getAndSet(future, update); - } - - @Override - @Nullable Waiter gasWaiters(AbstractFutureState future, Waiter update) { - return (Waiter) waitersUpdater.getAndSet(future, update); - } - - @Override - boolean casValue(AbstractFutureState future, @Nullable Object expect, Object update) { - return valueUpdater.compareAndSet(future, expect, update); - } - - private static LinkageError newLinkageError(Throwable cause) { - return new LinkageError(cause.toString(), cause); - } - - @Override - String atomicHelperTypeForTest() { - return "VarHandleAtomicHelper"; - } - } - /** * {@link AtomicHelper} based on {@link sun.misc.Unsafe}. * 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 b766e8659ede..b81c88ba9b86 100644 --- a/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java +++ b/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java @@ -89,8 +89,14 @@ public static TestSuite suite() { @Override public void runTest() throws Exception { - // First ensure that our classloaders are initializing the correct helper versions - if (isJava8() || isAndroid()) { + /* + * Note that we do not run this test under Android at the moment. For Android testing, see + * AbstractFutureDefaultAtomicHelperTest. + */ + + // First, ensure that our classloaders are initializing the correct helper versions: + + if (isJava8()) { checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper"); } else { checkHelperVersion(getClass().getClassLoader(), "VarHandleAtomicHelper"); @@ -99,12 +105,14 @@ public void runTest() throws Exception { checkHelperVersion(NO_UNSAFE, "AtomicReferenceFieldUpdaterAtomicHelper"); checkHelperVersion(NO_ATOMIC_REFERENCE_FIELD_UPDATER, "SynchronizedHelper"); + // Then, run the actual tests under each alternative classloader: + /* - * Under Java 8 or Android, there is no need to test the no-VarHandle case here: It's already - * tested by the main AbstractFutureTest, which uses the default AtomicHelper, which we verified - * above to be UnsafeAtomicHelper. + * Under Java 8, there is no need to test the no-VarHandle case here: It's already tested by the + * main AbstractFutureTest, which uses the default AtomicHelper, which we verified above to be + * UnsafeAtomicHelper. */ - if (!isJava8() && !isAndroid()) { + if (!isJava8()) { runTestMethod(NO_VAR_HANDLE); } @@ -164,8 +172,4 @@ public Class loadClass(String name) throws ClassNotFoundException { private static boolean isJava8() { return JAVA_SPECIFICATION_VERSION.value().equals("1.8"); } - - private static boolean isAndroid() { - return System.getProperty("java.runtime.name", "").contains("Android"); - } } 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 e9d45d645f77..6c6729b2fcfd 100644 --- a/guava-tests/test/com/google/common/util/concurrent/AggregateFutureStateFallbackAtomicHelperTest.java +++ b/guava-tests/test/com/google/common/util/concurrent/AggregateFutureStateFallbackAtomicHelperTest.java @@ -86,10 +86,18 @@ public static TestSuite suite() { @Override public void runTest() throws Exception { - // First ensure that our classloaders are initializing the correct helper versions + /* + * Note that we do not run this test under Android at the moment. For Android testing, see + * AggregateFutureStateDefaultAtomicHelperTest. + */ + + // First, ensure that our classloaders are initializing the correct helper versions: + checkHelperVersion(getClass().getClassLoader(), "SafeAtomicHelper"); checkHelperVersion(NO_ATOMIC_FIELD_UPDATER, "SynchronizedAtomicHelper"); + // Then, run the actual tests under each alternative classloader: + runTestMethod(NO_ATOMIC_FIELD_UPDATER); // TODO(lukes): assert that the logs are full of errors } diff --git a/guava/src/com/google/common/util/concurrent/AbstractFutureState.java b/guava/src/com/google/common/util/concurrent/AbstractFutureState.java index 9c02803429d6..1f0b61890367 100644 --- a/guava/src/com/google/common/util/concurrent/AbstractFutureState.java +++ b/guava/src/com/google/common/util/concurrent/AbstractFutureState.java @@ -403,7 +403,7 @@ void unpark() { * lookup would fail with an IllegalAccessException. That may then trigger use of Unsafe (possibly * with a warning under recent JVMs), or it may fall back even further to * AtomicReferenceFieldUpdaterAtomicHelper, which would fail with a similar problem to - * VarHandleAtomicHelperMaker, forcing us all the way to SynchronizedAtomicHelper. + * VarHandleAtomicHelperMaker, forcing us all the way to SynchronizedHelper. * * Additionally, it seems that nestmates do not help with runtime reflection under *Android*, even * when we use a newer -source and -target. That doesn't normally matter for AbstractFutureState, diff --git a/proguard/concurrent.pro b/proguard/concurrent.pro index 95a0f762defd..bb7b934b9370 100644 --- a/proguard/concurrent.pro +++ b/proguard/concurrent.pro @@ -47,10 +47,3 @@ -keep class com.google.apphosting.api.ApiProxy { static *** getCurrentEnvironment (...); } - -# b/407533570 --dontwarn com.google.common.util.concurrent.AbstractFutureState$VarHandleAtomicHelper -# See post-submit commentary on cl/743198569 about b/408047495. --assumevalues class com.google.common.util.concurrent.AbstractFutureState { - boolean mightBeAndroid() return true; -}