Skip to content

Commit 25b2277

Browse files
cpovirkGoogle Java Core Libraries
authored and
Google Java Core Libraries
committed
Make AbstractFuture use VarHandle when available.
For now, this is only for the JRE flavor, not for Android. Our not entirely great benchmarks suggest that this may actually improve performance slightly over the current `Unsafe`-based implementation. This matches my sense that [alternatives to `Unsafe` have gotten faster](#6806 (comment)). There are plenty of other [places in Guava that we use `Unsafe`](#6806), but this is a start. Also: I'm going to consider this CL to "fix" #6549, even though: - We already started requiring newer versions of Java to build our _tests_ in cl/705512728. (This CL is the first to require a newer JDK to build _prod_ code, again only to _build_ it, not to _run_ it.) - This CL requires only Java 9, not Java 11. - None of the changes so far should require people who _build our Maven project_ to do anything, since our build automatically downloads a new JDK to use for javac since cl/655647768. RELNOTES=n/a PiperOrigin-RevId: 702770479
1 parent e25ee0c commit 25b2277

File tree

3 files changed

+209
-53
lines changed

3 files changed

+209
-53
lines changed

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

+13-12
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
package com.google.common.util.concurrent;
1616

17+
import static com.google.common.base.StandardSystemProperty.JAVA_SPECIFICATION_VERSION;
18+
1719
import com.google.common.collect.ImmutableSet;
1820
import java.lang.reflect.Field;
1921
import java.lang.reflect.Method;
@@ -31,18 +33,12 @@
3133
* <p>On different platforms AbstractFuture uses different strategies for its core synchronization
3234
* primitives. The strategies are all implemented as subtypes of AtomicHelper and the strategy is
3335
* selected in the static initializer of AbstractFuture. This is convenient and performant but
34-
* introduces some testing difficulties. This test exercises the two fallback strategies in abstract
35-
* future.
36-
*
37-
* <ul>
38-
* <li>SafeAtomicHelper: uses AtomicReferenceFieldsUpdaters to implement synchronization
39-
* <li>SynchronizedHelper: uses {@code synchronized} blocks for synchronization
40-
* </ul>
36+
* introduces some testing difficulties. This test exercises the fallback strategies.
4137
*
42-
* To force selection of our fallback strategies we load {@link AbstractFuture} (and all of {@code
43-
* com.google.common.util.concurrent}) in degenerate class loaders which make certain platform
44-
* classes unavailable. Then we construct a test suite so we can run the normal AbstractFutureTest
45-
* test methods in these degenerate classloaders.
38+
* <p>To force selection of our fallback strategies, we load {@link AbstractFuture} (and all of
39+
* {@code com.google.common.util.concurrent}) in degenerate class loaders which make certain
40+
* platform classes unavailable. Then we construct a test suite so we can run the normal
41+
* AbstractFutureTest test methods in these degenerate classloaders.
4642
*/
4743

4844
public class AbstractFutureFallbackAtomicHelperTest extends TestCase {
@@ -60,7 +56,8 @@ public class AbstractFutureFallbackAtomicHelperTest extends TestCase {
6056

6157
/**
6258
* This classloader disallows {@link sun.misc.Unsafe} and {@link AtomicReferenceFieldUpdater},
63-
* which will prevent us from selecting our {@code SafeAtomicHelper} strategy.
59+
* which will prevent us from selecting our {@code AtomicReferenceFieldUpdaterAtomicHelper}
60+
* strategy.
6461
*/
6562
@SuppressWarnings({"SunApi", "removal"}) // b/345822163
6663
private static final ClassLoader NO_ATOMIC_REFERENCE_FIELD_UPDATER =
@@ -142,4 +139,8 @@ public Class<?> loadClass(String name) throws ClassNotFoundException {
142139
}
143140
};
144141
}
142+
143+
private static boolean isJava8() {
144+
return JAVA_SPECIFICATION_VERSION.value().equals("1.8");
145+
}
145146
}

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

+42-18
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
package com.google.common.util.concurrent;
1616

17+
import static com.google.common.base.StandardSystemProperty.JAVA_SPECIFICATION_VERSION;
18+
1719
import com.google.common.collect.ImmutableSet;
1820
import java.lang.reflect.Field;
1921
import java.lang.reflect.Method;
@@ -31,18 +33,12 @@
3133
* <p>On different platforms AbstractFuture uses different strategies for its core synchronization
3234
* primitives. The strategies are all implemented as subtypes of AtomicHelper and the strategy is
3335
* selected in the static initializer of AbstractFuture. This is convenient and performant but
34-
* introduces some testing difficulties. This test exercises the two fallback strategies in abstract
35-
* future.
36-
*
37-
* <ul>
38-
* <li>SafeAtomicHelper: uses AtomicReferenceFieldsUpdaters to implement synchronization
39-
* <li>SynchronizedHelper: uses {@code synchronized} blocks for synchronization
40-
* </ul>
36+
* introduces some testing difficulties. This test exercises the fallback strategies.
4137
*
42-
* To force selection of our fallback strategies we load {@link AbstractFuture} (and all of {@code
43-
* com.google.common.util.concurrent}) in degenerate class loaders which make certain platform
44-
* classes unavailable. Then we construct a test suite so we can run the normal AbstractFutureTest
45-
* test methods in these degenerate classloaders.
38+
* <p>To force selection of our fallback strategies, we load {@link AbstractFuture} (and all of
39+
* {@code com.google.common.util.concurrent}) in degenerate class loaders which make certain
40+
* platform classes unavailable. Then we construct a test suite so we can run the normal
41+
* AbstractFutureTest test methods in these degenerate classloaders.
4642
*/
4743

4844
public class AbstractFutureFallbackAtomicHelperTest extends TestCase {
@@ -51,21 +47,33 @@ public class AbstractFutureFallbackAtomicHelperTest extends TestCase {
5147
// execution significantly)
5248

5349
/**
54-
* This classloader disallows {@link sun.misc.Unsafe}, which will prevent us from selecting our
55-
* preferred strategy {@code UnsafeAtomicHelper}.
50+
* This classloader disallows {@link java.lang.invoke.VarHandle}, which will prevent us from
51+
* selecting our preferred strategy {@code VarHandleAtomicHelper}.
52+
*/
53+
@SuppressWarnings({"SunApi", "removal"}) // b/345822163
54+
private static final ClassLoader NO_VAR_HANDLE =
55+
getClassLoader(ImmutableSet.of("java.lang.invoke.VarHandle"));
56+
57+
/**
58+
* This classloader disallows {@link java.lang.invoke.VarHandle} and {@link sun.misc.Unsafe},
59+
* which will prevent us from selecting our {@code UnsafeAtomicHelper} strategy.
5660
*/
5761
@SuppressWarnings({"SunApi", "removal"}) // b/345822163
5862
private static final ClassLoader NO_UNSAFE =
59-
getClassLoader(ImmutableSet.of(Unsafe.class.getName()));
63+
getClassLoader(ImmutableSet.of("java.lang.invoke.VarHandle", Unsafe.class.getName()));
6064

6165
/**
62-
* This classloader disallows {@link sun.misc.Unsafe} and {@link AtomicReferenceFieldUpdater},
63-
* which will prevent us from selecting our {@code SafeAtomicHelper} strategy.
66+
* This classloader disallows {@link java.lang.invoke.VarHandle}, {@link sun.misc.Unsafe} and
67+
* {@link AtomicReferenceFieldUpdater}, which will prevent us from selecting our {@code
68+
* AtomicReferenceFieldUpdaterAtomicHelper} strategy.
6469
*/
6570
@SuppressWarnings({"SunApi", "removal"}) // b/345822163
6671
private static final ClassLoader NO_ATOMIC_REFERENCE_FIELD_UPDATER =
6772
getClassLoader(
68-
ImmutableSet.of(Unsafe.class.getName(), AtomicReferenceFieldUpdater.class.getName()));
73+
ImmutableSet.of(
74+
"java.lang.invoke.VarHandle",
75+
Unsafe.class.getName(),
76+
AtomicReferenceFieldUpdater.class.getName()));
6977

7078
public static TestSuite suite() {
7179
// we create a test suite containing a test for every AbstractFutureTest test method and we
@@ -84,13 +92,25 @@ public static TestSuite suite() {
8492
@Override
8593
public void runTest() throws Exception {
8694
// First ensure that our classloaders are initializing the correct helper versions
87-
checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper");
95+
if (isJava8()) {
96+
checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper");
97+
} else {
98+
checkHelperVersion(getClass().getClassLoader(), "VarHandleAtomicHelper");
99+
}
100+
checkHelperVersion(NO_VAR_HANDLE, "UnsafeAtomicHelper");
88101
checkHelperVersion(NO_UNSAFE, "AtomicReferenceFieldUpdaterAtomicHelper");
89102
checkHelperVersion(NO_ATOMIC_REFERENCE_FIELD_UPDATER, "SynchronizedHelper");
90103

91104
// Run the corresponding AbstractFutureTest test method in a new classloader that disallows
92105
// certain core jdk classes.
93106
ClassLoader oldClassLoader = Thread.currentThread().getContextClassLoader();
107+
Thread.currentThread().setContextClassLoader(NO_UNSAFE);
108+
try {
109+
runTestMethod(NO_VAR_HANDLE);
110+
} finally {
111+
Thread.currentThread().setContextClassLoader(oldClassLoader);
112+
}
113+
94114
Thread.currentThread().setContextClassLoader(NO_UNSAFE);
95115
try {
96116
runTestMethod(NO_UNSAFE);
@@ -142,4 +162,8 @@ public Class<?> loadClass(String name) throws ClassNotFoundException {
142162
}
143163
};
144164
}
165+
166+
private static boolean isJava8() {
167+
return JAVA_SPECIFICATION_VERSION.value().equals("1.8");
168+
}
145169
}

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

+154-23
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,11 @@
2929
import com.google.common.util.concurrent.internal.InternalFutures;
3030
import com.google.errorprone.annotations.CanIgnoreReturnValue;
3131
import com.google.errorprone.annotations.ForOverride;
32+
import com.google.j2objc.annotations.J2ObjCIncompatible;
3233
import com.google.j2objc.annotations.ReflectionSupport;
3334
import com.google.j2objc.annotations.RetainedLocalRef;
35+
import java.lang.invoke.MethodHandles;
36+
import java.lang.invoke.VarHandle;
3437
import java.lang.reflect.Field;
3538
import java.security.AccessController;
3639
import java.security.PrivilegedActionException;
@@ -152,35 +155,60 @@ public final boolean cancel(boolean mayInterruptIfRunning) {
152155

153156
private static final AtomicHelper ATOMIC_HELPER;
154157

158+
/**
159+
* Returns the result of calling {@link MethodHandles#lookup} from inside {@link AbstractFuture}.
160+
* By virtue of being created there, it has access to the private fields of {@link
161+
* AbstractFuture}, so that access is available to anyone who calls this method—specifically, to
162+
* {@link VarHandleAtomicHelper}.
163+
*
164+
* <p>This "shouldn't" be necessary: {@link VarHandleAtomicHelper} and {@link AbstractFuture}
165+
* "should" be nestmates, so a call to {@link MethodHandles#lookup} inside {@link
166+
* VarHandleAtomicHelper} "should" have access to each other's private fields. However, our
167+
* open-source build uses {@code -source 8 -target 8}, so the class files from that build can't
168+
* express nestmates. Thus, when those class files are used from Java 9 or higher (i.e., high
169+
* enough to trigger the {@link VarHandle} code path), such a lookup would fail with an {@link
170+
* IllegalAccessException}.
171+
*
172+
* <p>Note that we do not have a similar problem with the fields in {@link Waiter} because those
173+
* fields are not private. (We could solve the problem with {@link AbstractFuture} fields in the
174+
* same way if we wanted.)
175+
*/
176+
private static MethodHandles.Lookup methodHandlesLookupFromWithinAbstractFuture() {
177+
return MethodHandles.lookup();
178+
}
179+
155180
static {
156181
AtomicHelper helper;
157182
Throwable thrownUnsafeFailure = null;
158183
Throwable thrownAtomicReferenceFieldUpdaterFailure = null;
159184

160-
try {
161-
helper = new UnsafeAtomicHelper();
162-
} catch (Exception | Error unsafeFailure) { // sneaky checked exception
163-
thrownUnsafeFailure = unsafeFailure;
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
185+
helper = VarHandleAtomicHelperMaker.INSTANCE.tryMakeVarHandleAtomicHelper();
186+
if (helper == null) {
168187
try {
169-
helper =
170-
new AtomicReferenceFieldUpdaterAtomicHelper(
171-
newUpdater(Waiter.class, Thread.class, "thread"),
172-
newUpdater(Waiter.class, Waiter.class, "next"),
173-
newUpdater(AbstractFuture.class, Waiter.class, "waiters"),
174-
newUpdater(AbstractFuture.class, Listener.class, "listeners"),
175-
newUpdater(AbstractFuture.class, Object.class, "value"));
176-
} catch (Exception // sneaky checked exception
177-
| Error atomicReferenceFieldUpdaterFailure) {
178-
// Some Android 5.0.x Samsung devices have bugs in JDK reflection APIs that cause
179-
// getDeclaredField to throw a NoSuchFieldException when the field is definitely there.
180-
// For these users fallback to a suboptimal implementation, based on synchronized. This will
181-
// be a definite performance hit to those users.
182-
thrownAtomicReferenceFieldUpdaterFailure = atomicReferenceFieldUpdaterFailure;
183-
helper = new SynchronizedHelper();
188+
helper = new UnsafeAtomicHelper();
189+
} catch (Exception | Error unsafeFailure) { // sneaky checked exception
190+
thrownUnsafeFailure = unsafeFailure;
191+
// catch absolutely everything and fall through to our
192+
// 'AtomicReferenceFieldUpdaterAtomicHelper' The access control checks that ARFU does means
193+
// the caller class has to be AbstractFuture instead of
194+
// AtomicReferenceFieldUpdaterAtomicHelper, so we annoyingly define these here
195+
try {
196+
helper =
197+
new AtomicReferenceFieldUpdaterAtomicHelper(
198+
newUpdater(Waiter.class, Thread.class, "thread"),
199+
newUpdater(Waiter.class, Waiter.class, "next"),
200+
newUpdater(AbstractFuture.class, Waiter.class, "waiters"),
201+
newUpdater(AbstractFuture.class, Listener.class, "listeners"),
202+
newUpdater(AbstractFuture.class, Object.class, "value"));
203+
} catch (Exception // sneaky checked exception
204+
| Error atomicReferenceFieldUpdaterFailure) {
205+
// Some Android 5.0.x Samsung devices have bugs in JDK reflection APIs that cause
206+
// getDeclaredField to throw a NoSuchFieldException when the field is definitely there.
207+
// For these users fallback to a suboptimal implementation, based on synchronized. This
208+
// will be a definite performance hit to those users.
209+
thrownAtomicReferenceFieldUpdaterFailure = atomicReferenceFieldUpdaterFailure;
210+
helper = new SynchronizedHelper();
211+
}
184212
}
185213
}
186214
ATOMIC_HELPER = helper;
@@ -202,6 +230,42 @@ public final boolean cancel(boolean mayInterruptIfRunning) {
202230
}
203231
}
204232

233+
private enum VarHandleAtomicHelperMaker {
234+
INSTANCE {
235+
/**
236+
* Implementation used by non-J2ObjC environments (aside, of course, from those that have
237+
* supersource for the entirety of {@link AbstractFuture}).
238+
*/
239+
@Override
240+
@J2ObjCIncompatible
241+
@CheckForNull
242+
AtomicHelper tryMakeVarHandleAtomicHelper() {
243+
try {
244+
/*
245+
* We first use reflection to check whether VarHandle exists. If we instead just tried to
246+
* load our class directly (which would trigger non-reflective loading of VarHandle) from
247+
* within a `try` block, then an error might be thrown even before we enter the `try`
248+
* block: https://github.com/google/truth/issues/333#issuecomment-765652454
249+
*
250+
* Also, it's nice that this approach should let us catch *only* ClassNotFoundException
251+
* instead of having to catch more broadly (potentially even including, say, a
252+
* StackOverflowError).
253+
*/
254+
Class.forName("java.lang.invoke.VarHandle");
255+
} catch (ClassNotFoundException beforeJava9) {
256+
return null;
257+
}
258+
return new VarHandleAtomicHelper();
259+
}
260+
};
261+
262+
/** Implementation used by J2ObjC environments, overridden for other environments. */
263+
@CheckForNull
264+
AtomicHelper tryMakeVarHandleAtomicHelper() {
265+
return null;
266+
}
267+
}
268+
205269
/** Waiter links form a Treiber stack, in the {@link #waiters} field. */
206270
private static final class Waiter {
207271
static final Waiter TOMBSTONE = new Waiter(false /* ignored param */);
@@ -1344,6 +1408,73 @@ abstract boolean casListeners(
13441408
abstract boolean casValue(AbstractFuture<?> future, @CheckForNull Object expect, Object update);
13451409
}
13461410

1411+
/** {@link AtomicHelper} based on {@link VarHandle}. */
1412+
@J2ObjCIncompatible
1413+
// We use this class only after confirming that VarHandle is available at runtime.
1414+
@SuppressWarnings("Java8ApiChecker")
1415+
@IgnoreJRERequirement
1416+
private static final class VarHandleAtomicHelper extends AtomicHelper {
1417+
static final VarHandle waiterThreadUpdater;
1418+
static final VarHandle waiterNextUpdater;
1419+
static final VarHandle waitersUpdater;
1420+
static final VarHandle listenersUpdater;
1421+
static final VarHandle valueUpdater;
1422+
1423+
static {
1424+
MethodHandles.Lookup lookup = methodHandlesLookupFromWithinAbstractFuture();
1425+
try {
1426+
waiterThreadUpdater = lookup.findVarHandle(Waiter.class, "thread", Thread.class);
1427+
waiterNextUpdater = lookup.findVarHandle(Waiter.class, "next", Waiter.class);
1428+
waitersUpdater = lookup.findVarHandle(AbstractFuture.class, "waiters", Waiter.class);
1429+
listenersUpdater = lookup.findVarHandle(AbstractFuture.class, "listeners", Listener.class);
1430+
valueUpdater = lookup.findVarHandle(AbstractFuture.class, "value", Object.class);
1431+
} catch (ReflectiveOperationException e) {
1432+
// Those fields exist.
1433+
throw newLinkageError(e);
1434+
}
1435+
}
1436+
1437+
@Override
1438+
void putThread(Waiter waiter, Thread newValue) {
1439+
waiterThreadUpdater.setRelease(waiter, newValue);
1440+
}
1441+
1442+
@Override
1443+
void putNext(Waiter waiter, @CheckForNull Waiter newValue) {
1444+
waiterNextUpdater.setRelease(waiter, newValue);
1445+
}
1446+
1447+
@Override
1448+
boolean casWaiters(
1449+
AbstractFuture<?> future, @CheckForNull Waiter expect, @CheckForNull Waiter update) {
1450+
return waitersUpdater.compareAndSet(future, expect, update);
1451+
}
1452+
1453+
@Override
1454+
boolean casListeners(AbstractFuture<?> future, @CheckForNull Listener expect, Listener update) {
1455+
return listenersUpdater.compareAndSet(future, expect, update);
1456+
}
1457+
1458+
@Override
1459+
Listener gasListeners(AbstractFuture<?> future, Listener update) {
1460+
return (Listener) listenersUpdater.getAndSet(future, update);
1461+
}
1462+
1463+
@Override
1464+
Waiter gasWaiters(AbstractFuture<?> future, Waiter update) {
1465+
return (Waiter) waitersUpdater.getAndSet(future, update);
1466+
}
1467+
1468+
@Override
1469+
boolean casValue(AbstractFuture<?> future, @CheckForNull Object expect, Object update) {
1470+
return valueUpdater.compareAndSet(future, expect, update);
1471+
}
1472+
1473+
private static LinkageError newLinkageError(Throwable cause) {
1474+
return new LinkageError(cause.toString(), cause);
1475+
}
1476+
}
1477+
13471478
/**
13481479
* {@link AtomicHelper} based on {@link sun.misc.Unsafe}.
13491480
*

0 commit comments

Comments
 (0)