Skip to content

Commit 7336af1

Browse files
cpovirkGoogle Java Core Libraries
cpovirk
authored and
Google Java Core Libraries
committed
Make the guava-android copy of AbstractFuture try VarHandle when run under the JVM.
Under Android, I still don't have us even _try_ `VarHandle`: - I'm not sure that using `VarHandle` is possible with our current internal Android build setup, as discussed in the internal commentary on cl/711733182. - Using `VarHandle` there may at least somewhat more complex: My testing suggests that some versions of Android expose `VarHandle` to our reflective check but don't actually expose `MethodHandles.Lookup.findVarHandle`. Accordingly, sgjesse@ [recommends using a check on `SDK_INT`](https://issuetracker.google.com/issues/134377851#comment4) ([33](https://developer.android.com/reference/java/lang/invoke/MethodHandles.Lookup#findVarHandle(java.lang.Class%3C?%3E,%20java.lang.String,%20java.lang.Class%3C?%3E)) or higher) instead of reflection under Android. Since `guava-android` can be used under the JVM, too, we'd need to use a combination of the SDK check _and_ reflection. And we'd need to perform the `SDK_INT` check reflectively, as [we do in `TempFileCreator`](https://github.com/google/guava/blob/266ce0022a1c54244df1af0bde81116ed7703577/guava/src/com/google/common/io/TempFileCreator.java#L75) (which, hmm, I should clean up as obsolete one of these days...). (We could at least _reduce_ the amount of reflection we need if we were to depend on the Android SDK at build time, as discussed in b/403282918.) - I have no idea whether `VarHandle` is faster or slower than `Unsafe` there (and I can't easily tell because of the build problems above). - I'm not aware of efforts to remove `Unsafe` under Android. This CL has two advantages for JVM users of `guava-android`: - It eliminates a warning about usage of `Unsafe` under newer JDKs. Note that `AbstractFuture` _already_ has run correctly if access to `Unsafe` is outright disallowed (as with `-sun-misc-unsafe-memory-access=deny`): It detects this and falls back to an alternative implementation. However, if `Unsafe` is available but produces warnings, `guava-android` would use it, triggering those warnings. This CL makes Guava not even try to use it under newer JVMs because it now tries `VarHandle` first. - `VarHandle` may be marginally faster than `Unsafe` under the JVM, as discussed in cl/711733182. (It also doesn't lead to VM crashes if you manage to pass `null` where you shouldn't, as I did back in b/397641020 :) But that's more something that's nice for us as Guava developers, not something that's nice for Guava _users_.) This CL is probably the most _prominent_ part of [our migration of `guava-android` off `Unsafe`](#7742). It is debatable whether it is the most _important_, given that [at least one class, `Striped64`, does not seem to have a fallback at all if `Unsafe` is unavailable](#6806 (comment)). Still, this CL addresses the warning that most users are seeing, and it gives us some precedent for how to deal with `Striped64`. Finally, it looks like our existing tests for `VarHandle` had [a mismatch between the context class loader and the class loader that we load `AbstractFutureTest` in](https://github.com/google/guava/blob/266ce0022a1c54244df1af0bde81116ed7703577/guava-tests/test/com/google/common/util/concurrent/AbstractFutureFallbackAtomicHelperTest.java#L105-L107)? That seems fairly bad. This CL fixes it, extracting a method to guard against future such mismatches. Out of an abundance of caution, I made a similar change in `AggregateFutureStateFallbackAtomicHelperTest`, even though there's not really an opportunity for a mismatch there, given that there's only one alternative class loader. RELNOTES=`util.concurrent`: Changed the `guava-android` copy of `AbstractFuture` to try `VarHandle` before `Unsafe`, eliminating a warning under newer JDKs. PiperOrigin-RevId: 743198569
1 parent b4da629 commit 7336af1

9 files changed

Lines changed: 272 additions & 96 deletions

File tree

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,18 @@
3030
@NullUnmarked
3131
public class AbstractFutureDefaultAtomicHelperTest extends TestCase {
3232
public void testUsingExpectedAtomicHelper() throws Exception {
33-
assertThat(AbstractFutureState.atomicHelperTypeForTest()).isEqualTo("UnsafeAtomicHelper");
33+
if (isJava8() || isAndroid()) {
34+
assertThat(AbstractFutureState.atomicHelperTypeForTest()).isEqualTo("UnsafeAtomicHelper");
35+
} else {
36+
assertThat(AbstractFutureState.atomicHelperTypeForTest()).isEqualTo("VarHandleAtomicHelper");
37+
}
3438
}
3539

3640
private static boolean isJava8() {
3741
return JAVA_SPECIFICATION_VERSION.value().equals("1.8");
3842
}
43+
44+
private static boolean isAndroid() {
45+
return System.getProperty("java.runtime.name", "").contains("Android");
46+
}
3947
}

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

Lines changed: 51 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -48,19 +48,30 @@ public class AbstractFutureFallbackAtomicHelperTest extends TestCase {
4848
// execution significantly)
4949

5050
/**
51-
* This classloader disallows {@link sun.misc.Unsafe}, which will prevent us from selecting our
52-
* preferred strategy {@code UnsafeAtomicHelper}.
51+
* This classloader disallows {@link java.lang.invoke.VarHandle}, which will prevent us from
52+
* selecting the {@code VarHandleAtomicHelper} strategy.
5353
*/
54-
private static final ClassLoader NO_UNSAFE = getClassLoader(ImmutableSet.of("sun.misc.Unsafe"));
54+
private static final ClassLoader NO_VAR_HANDLE =
55+
getClassLoader(ImmutableSet.of("java.lang.invoke.VarHandle"));
5556

5657
/**
57-
* This classloader disallows {@link sun.misc.Unsafe} and {@link AtomicReferenceFieldUpdater},
58-
* which will prevent us from selecting our {@code AtomicReferenceFieldUpdaterAtomicHelper}
59-
* strategy.
58+
* This classloader disallows {@link java.lang.invoke.VarHandle} and {@link sun.misc.Unsafe},
59+
* which will prevent us from selecting the {@code UnsafeAtomicHelper} strategy.
60+
*/
61+
private static final ClassLoader NO_UNSAFE =
62+
getClassLoader(ImmutableSet.of("java.lang.invoke.VarHandle", "sun.misc.Unsafe"));
63+
64+
/**
65+
* This classloader disallows {@link java.lang.invoke.VarHandle}, {@link sun.misc.Unsafe} and
66+
* {@link AtomicReferenceFieldUpdater}, which will prevent us from selecting the {@code
67+
* AtomicReferenceFieldUpdaterAtomicHelper} strategy.
6068
*/
6169
private static final ClassLoader NO_ATOMIC_REFERENCE_FIELD_UPDATER =
6270
getClassLoader(
63-
ImmutableSet.of("sun.misc.Unsafe", AtomicReferenceFieldUpdater.class.getName()));
71+
ImmutableSet.of(
72+
"java.lang.invoke.VarHandle",
73+
"sun.misc.Unsafe",
74+
AtomicReferenceFieldUpdater.class.getName()));
6475

6576
public static TestSuite suite() {
6677
// we create a test suite containing a test for every AbstractFutureTest test method and we
@@ -79,34 +90,46 @@ public static TestSuite suite() {
7990
@Override
8091
public void runTest() throws Exception {
8192
// First ensure that our classloaders are initializing the correct helper versions
82-
checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper");
93+
if (isJava8() || isAndroid()) {
94+
checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper");
95+
} else {
96+
checkHelperVersion(getClass().getClassLoader(), "VarHandleAtomicHelper");
97+
}
98+
checkHelperVersion(NO_VAR_HANDLE, "UnsafeAtomicHelper");
8399
checkHelperVersion(NO_UNSAFE, "AtomicReferenceFieldUpdaterAtomicHelper");
84100
checkHelperVersion(NO_ATOMIC_REFERENCE_FIELD_UPDATER, "SynchronizedHelper");
85101

86-
// Run the corresponding AbstractFutureTest test method in a new classloader that disallows
87-
// certain core jdk classes.
88-
ClassLoader oldClassLoader = Thread.currentThread().getContextClassLoader();
89-
Thread.currentThread().setContextClassLoader(NO_UNSAFE);
90-
try {
91-
runTestMethod(NO_UNSAFE);
92-
} finally {
93-
Thread.currentThread().setContextClassLoader(oldClassLoader);
102+
/*
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.
106+
*/
107+
if (!isJava8() && !isAndroid()) {
108+
runTestMethod(NO_VAR_HANDLE);
94109
}
95110

96-
Thread.currentThread().setContextClassLoader(NO_ATOMIC_REFERENCE_FIELD_UPDATER);
111+
runTestMethod(NO_UNSAFE);
112+
113+
runTestMethod(NO_ATOMIC_REFERENCE_FIELD_UPDATER);
114+
// TODO(lukes): assert that the logs are full of errors
115+
}
116+
117+
/**
118+
* Runs the corresponding {@link AbstractFutureTest} test method in a new classloader that
119+
* disallows certain core JDK classes.
120+
*/
121+
private void runTestMethod(ClassLoader classLoader) throws Exception {
122+
ClassLoader oldClassLoader = Thread.currentThread().getContextClassLoader();
123+
124+
Thread.currentThread().setContextClassLoader(classLoader);
97125
try {
98-
runTestMethod(NO_ATOMIC_REFERENCE_FIELD_UPDATER);
99-
// TODO(lukes): assert that the logs are full of errors
126+
Class<?> test = classLoader.loadClass(AbstractFutureTest.class.getName());
127+
test.getMethod(getName()).invoke(test.getDeclaredConstructor().newInstance());
100128
} finally {
101129
Thread.currentThread().setContextClassLoader(oldClassLoader);
102130
}
103131
}
104132

105-
private void runTestMethod(ClassLoader classLoader) throws Exception {
106-
Class<?> test = classLoader.loadClass(AbstractFutureTest.class.getName());
107-
test.getMethod(getName()).invoke(test.getDeclaredConstructor().newInstance());
108-
}
109-
110133
private void checkHelperVersion(ClassLoader classLoader, String expectedHelperClassName)
111134
throws Exception {
112135
// Make sure we are actually running with the expected helper implementation
@@ -141,4 +164,8 @@ public Class<?> loadClass(String name) throws ClassNotFoundException {
141164
private static boolean isJava8() {
142165
return JAVA_SPECIFICATION_VERSION.value().equals("1.8");
143166
}
167+
168+
private static boolean isAndroid() {
169+
return System.getProperty("java.runtime.name", "").contains("Android");
170+
}
144171
}

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

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,9 @@
5050
public class AggregateFutureStateFallbackAtomicHelperTest extends TestCase {
5151

5252
/**
53-
* This classloader disallows AtomicReferenceFieldUpdater and AtomicIntegerFieldUpdate which will
54-
* prevent us from selecting our {@code SafeAtomicHelper} strategy.
53+
* This classloader disallows {@code AtomicReferenceFieldUpdater} and {@code
54+
* AtomicIntegerFieldUpdater}, which will prevent us from selecting the {@code SafeAtomicHelper}
55+
* strategy.
5556
*
5657
* <p>Stashing this in a static field avoids loading it over and over again and speeds up test
5758
* execution significantly.
@@ -89,26 +90,28 @@ public void runTest() throws Exception {
8990
checkHelperVersion(getClass().getClassLoader(), "SafeAtomicHelper");
9091
checkHelperVersion(NO_ATOMIC_FIELD_UPDATER, "SynchronizedAtomicHelper");
9192

92-
// Run the corresponding FuturesTest test method in a new classloader that disallows
93-
// certain core jdk classes.
93+
runTestMethod(NO_ATOMIC_FIELD_UPDATER);
94+
// TODO(lukes): assert that the logs are full of errors
95+
}
96+
97+
/**
98+
* Runs the corresponding {@link FuturesTest} test method in a new classloader that disallows
99+
* certain core JDK classes.
100+
*/
101+
private void runTestMethod(ClassLoader classLoader) throws Exception {
94102
ClassLoader oldClassLoader = Thread.currentThread().getContextClassLoader();
95-
Thread.currentThread().setContextClassLoader(NO_ATOMIC_FIELD_UPDATER);
103+
Thread.currentThread().setContextClassLoader(classLoader);
96104
try {
97-
runTestMethod(NO_ATOMIC_FIELD_UPDATER);
98-
// TODO(lukes): assert that the logs are full of errors
105+
Class<?> test = classLoader.loadClass(FuturesTest.class.getName());
106+
Object testInstance = test.getDeclaredConstructor().newInstance();
107+
test.getMethod("setUp").invoke(testInstance);
108+
test.getMethod(getName()).invoke(testInstance);
109+
test.getMethod("tearDown").invoke(testInstance);
99110
} finally {
100111
Thread.currentThread().setContextClassLoader(oldClassLoader);
101112
}
102113
}
103114

104-
private void runTestMethod(ClassLoader classLoader) throws Exception {
105-
Class<?> test = classLoader.loadClass(FuturesTest.class.getName());
106-
Object testInstance = test.getDeclaredConstructor().newInstance();
107-
test.getMethod("setUp").invoke(testInstance);
108-
test.getMethod(getName()).invoke(testInstance);
109-
test.getMethod("tearDown").invoke(testInstance);
110-
}
111-
112115
private void checkHelperVersion(ClassLoader classLoader, String expectedHelperClassName)
113116
throws Exception {
114117
// Make sure we are actually running with the expected helper implementation

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

Lines changed: 138 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,11 @@
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;
3031
import com.google.j2objc.annotations.ReflectionSupport;
3132
import com.google.j2objc.annotations.RetainedLocalRef;
33+
import java.lang.invoke.MethodHandles;
34+
import java.lang.invoke.VarHandle;
3235
import java.lang.reflect.Field;
3336
import java.security.PrivilegedActionException;
3437
import java.security.PrivilegedExceptionAction;
@@ -345,21 +348,24 @@ void unpark() {
345348
Throwable thrownUnsafeFailure = null;
346349
Throwable thrownAtomicReferenceFieldUpdaterFailure = null;
347350

348-
try {
349-
helper = new UnsafeAtomicHelper();
350-
} catch (Exception | Error unsafeFailure) { // sneaky checked exception
351-
thrownUnsafeFailure = unsafeFailure;
352-
// Catch absolutely everything and fall through to AtomicReferenceFieldUpdaterAtomicHelper.
351+
helper = VarHandleAtomicHelperMaker.INSTANCE.tryMakeVarHandleAtomicHelper();
352+
if (helper == null) {
353353
try {
354-
helper = new AtomicReferenceFieldUpdaterAtomicHelper();
355-
} catch (Exception // sneaky checked exception
356-
| Error atomicReferenceFieldUpdaterFailure) {
357-
// Some Android 5.0.x Samsung devices have bugs in JDK reflection APIs that cause
358-
// getDeclaredField to throw a NoSuchFieldException when the field is definitely there.
359-
// For these users fallback to a suboptimal implementation, based on synchronized. This
360-
// will be a definite performance hit to those users.
361-
thrownAtomicReferenceFieldUpdaterFailure = atomicReferenceFieldUpdaterFailure;
362-
helper = new SynchronizedHelper();
354+
helper = new UnsafeAtomicHelper();
355+
} catch (Exception | Error unsafeFailure) { // sneaky checked exception
356+
thrownUnsafeFailure = unsafeFailure;
357+
// Catch absolutely everything and fall through to AtomicReferenceFieldUpdaterAtomicHelper.
358+
try {
359+
helper = new AtomicReferenceFieldUpdaterAtomicHelper();
360+
} catch (Exception // sneaky checked exception
361+
| Error atomicReferenceFieldUpdaterFailure) {
362+
// Some Android 5.0.x Samsung devices have bugs in JDK reflection APIs that cause
363+
// getDeclaredField to throw a NoSuchFieldException when the field is definitely there.
364+
// For these users fallback to a suboptimal implementation, based on synchronized. This
365+
// will be a definite performance hit to those users.
366+
thrownAtomicReferenceFieldUpdaterFailure = atomicReferenceFieldUpdaterFailure;
367+
helper = new SynchronizedHelper();
368+
}
363369
}
364370
}
365371
ATOMIC_HELPER = helper;
@@ -496,6 +502,43 @@ static String atomicHelperTypeForTest() {
496502
return ATOMIC_HELPER.atomicHelperTypeForTest();
497503
}
498504

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

570+
/** {@link AtomicHelper} based on {@link VarHandle}. */
571+
@J2ObjCIncompatible
572+
// We use this class only after confirming that VarHandle is available at runtime.
573+
@SuppressWarnings({"Java8ApiChecker", "Java7ApiChecker", "AndroidJdkLibsChecker"})
574+
@IgnoreJRERequirement
575+
private static final class VarHandleAtomicHelper extends AtomicHelper {
576+
static final VarHandle waiterThreadUpdater;
577+
static final VarHandle waiterNextUpdater;
578+
static final VarHandle waitersUpdater;
579+
static final VarHandle listenersUpdater;
580+
static final VarHandle valueUpdater;
581+
582+
static {
583+
MethodHandles.Lookup lookup = MethodHandles.lookup();
584+
try {
585+
waiterThreadUpdater = lookup.findVarHandle(Waiter.class, "thread", Thread.class);
586+
waiterNextUpdater = lookup.findVarHandle(Waiter.class, "next", Waiter.class);
587+
waitersUpdater =
588+
lookup.findVarHandle(AbstractFutureState.class, "waitersField", Waiter.class);
589+
listenersUpdater =
590+
lookup.findVarHandle(AbstractFutureState.class, "listenersField", Listener.class);
591+
valueUpdater = lookup.findVarHandle(AbstractFutureState.class, "valueField", Object.class);
592+
} catch (ReflectiveOperationException e) {
593+
// Those fields exist.
594+
throw newLinkageError(e);
595+
}
596+
}
597+
598+
@Override
599+
void putThread(Waiter waiter, Thread newValue) {
600+
waiterThreadUpdater.setRelease(waiter, newValue);
601+
}
602+
603+
@Override
604+
void putNext(Waiter waiter, @Nullable Waiter newValue) {
605+
waiterNextUpdater.setRelease(waiter, newValue);
606+
}
607+
608+
@Override
609+
boolean casWaiters(
610+
AbstractFutureState<?> future, @Nullable Waiter expect, @Nullable Waiter update) {
611+
return waitersUpdater.compareAndSet(future, expect, update);
612+
}
613+
614+
@Override
615+
boolean casListeners(
616+
AbstractFutureState<?> future, @Nullable Listener expect, Listener update) {
617+
return listenersUpdater.compareAndSet(future, expect, update);
618+
}
619+
620+
@Override
621+
@Nullable Listener gasListeners(AbstractFutureState<?> future, Listener update) {
622+
return (Listener) listenersUpdater.getAndSet(future, update);
623+
}
624+
625+
@Override
626+
@Nullable Waiter gasWaiters(AbstractFutureState<?> future, Waiter update) {
627+
return (Waiter) waitersUpdater.getAndSet(future, update);
628+
}
629+
630+
@Override
631+
boolean casValue(AbstractFutureState<?> future, @Nullable Object expect, Object update) {
632+
return valueUpdater.compareAndSet(future, expect, update);
633+
}
634+
635+
private static LinkageError newLinkageError(Throwable cause) {
636+
return new LinkageError(cause.toString(), cause);
637+
}
638+
639+
@Override
640+
String atomicHelperTypeForTest() {
641+
return "VarHandleAtomicHelper";
642+
}
643+
}
644+
527645
/**
528646
* {@link AtomicHelper} based on {@link sun.misc.Unsafe}.
529647
*
@@ -777,4 +895,10 @@ String atomicHelperTypeForTest() {
777895
return "SynchronizedHelper";
778896
}
779897
}
898+
899+
private static boolean mightBeAndroid() {
900+
String runtime = System.getProperty("java.runtime.name", "");
901+
// I have no reason to believe that `null` is possible here, but let's make sure we don't crash:
902+
return runtime == null || runtime.contains("Android");
903+
}
780904
}

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
@NullUnmarked
3131
public class AbstractFutureDefaultAtomicHelperTest extends TestCase {
3232
public void testUsingExpectedAtomicHelper() throws Exception {
33-
if (isJava8()) {
33+
if (isJava8() || isAndroid()) {
3434
assertThat(AbstractFutureState.atomicHelperTypeForTest()).isEqualTo("UnsafeAtomicHelper");
3535
} else {
3636
assertThat(AbstractFutureState.atomicHelperTypeForTest()).isEqualTo("VarHandleAtomicHelper");
@@ -40,4 +40,8 @@ public void testUsingExpectedAtomicHelper() throws Exception {
4040
private static boolean isJava8() {
4141
return JAVA_SPECIFICATION_VERSION.value().equals("1.8");
4242
}
43+
44+
private static boolean isAndroid() {
45+
return System.getProperty("java.runtime.name", "").contains("Android");
46+
}
4347
}

0 commit comments

Comments
 (0)