Skip to content

Commit

Permalink
Make AbstractFuture use VarHandle when available.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
cpovirk authored and Google Java Core Libraries committed Dec 17, 2024
1 parent e25ee0c commit 06f6188
Show file tree
Hide file tree
Showing 3 changed files with 204 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

package com.google.common.util.concurrent;

import static com.google.common.base.StandardSystemProperty.JAVA_SPECIFICATION_VERSION;

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

public class AbstractFutureFallbackAtomicHelperTest extends TestCase {
Expand All @@ -60,7 +56,8 @@ public class AbstractFutureFallbackAtomicHelperTest extends TestCase {

/**
* This classloader disallows {@link sun.misc.Unsafe} and {@link AtomicReferenceFieldUpdater},
* which will prevent us from selecting our {@code SafeAtomicHelper} strategy.
* which will prevent us from selecting our {@code AtomicReferenceFieldUpdaterAtomicHelper}
* strategy.
*/
@SuppressWarnings({"SunApi", "removal"}) // b/345822163
private static final ClassLoader NO_ATOMIC_REFERENCE_FIELD_UPDATER =
Expand Down Expand Up @@ -142,4 +139,8 @@ public Class<?> loadClass(String name) throws ClassNotFoundException {
}
};
}

private static boolean isJava8() {
return JAVA_SPECIFICATION_VERSION.value().equals("1.8");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

package com.google.common.util.concurrent;

import static com.google.common.base.StandardSystemProperty.JAVA_SPECIFICATION_VERSION;

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

public class AbstractFutureFallbackAtomicHelperTest extends TestCase {
Expand All @@ -51,21 +47,33 @@ public class AbstractFutureFallbackAtomicHelperTest extends TestCase {
// execution significantly)

/**
* This classloader disallows {@link sun.misc.Unsafe}, which will prevent us from selecting our
* preferred strategy {@code UnsafeAtomicHelper}.
* This classloader disallows {@link java.lang.invoke.VarHandle}, which will prevent us from
* selecting our preferred strategy {@code VarHandleAtomicHelper}.
*/
@SuppressWarnings({"SunApi", "removal"}) // b/345822163
private static final ClassLoader NO_VAR_HANDLE =
getClassLoader(ImmutableSet.of("java.lang.invoke.VarHandle"));

/**
* This classloader disallows {@link java.lang.invoke.VarHandle} and {@link sun.misc.Unsafe},
* which will prevent us from selecting our {@code UnsafeAtomicHelper} strategy.
*/
@SuppressWarnings({"SunApi", "removal"}) // b/345822163
private static final ClassLoader NO_UNSAFE =
getClassLoader(ImmutableSet.of(Unsafe.class.getName()));
getClassLoader(ImmutableSet.of("java.lang.invoke.VarHandle", Unsafe.class.getName()));

/**
* This classloader disallows {@link sun.misc.Unsafe} and {@link AtomicReferenceFieldUpdater},
* which will prevent us from selecting our {@code SafeAtomicHelper} strategy.
* This classloader disallows {@link java.lang.invoke.VarHandle}, {@link sun.misc.Unsafe} and
* {@link AtomicReferenceFieldUpdater}, which will prevent us from selecting our {@code
* AtomicReferenceFieldUpdaterAtomicHelper} strategy.
*/
@SuppressWarnings({"SunApi", "removal"}) // b/345822163
private static final ClassLoader NO_ATOMIC_REFERENCE_FIELD_UPDATER =
getClassLoader(
ImmutableSet.of(Unsafe.class.getName(), AtomicReferenceFieldUpdater.class.getName()));
ImmutableSet.of(
"java.lang.invoke.VarHandle",
Unsafe.class.getName(),
AtomicReferenceFieldUpdater.class.getName()));

public static TestSuite suite() {
// we create a test suite containing a test for every AbstractFutureTest test method and we
Expand All @@ -84,13 +92,25 @@ public static TestSuite suite() {
@Override
public void runTest() throws Exception {
// First ensure that our classloaders are initializing the correct helper versions
checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper");
if (isJava8()) {
checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper");
} else {
checkHelperVersion(getClass().getClassLoader(), "VarHandleAtomicHelper");
}
checkHelperVersion(NO_VAR_HANDLE, "UnsafeAtomicHelper");
checkHelperVersion(NO_UNSAFE, "AtomicReferenceFieldUpdaterAtomicHelper");
checkHelperVersion(NO_ATOMIC_REFERENCE_FIELD_UPDATER, "SynchronizedHelper");

// Run the corresponding AbstractFutureTest test method in a new classloader that disallows
// certain core jdk classes.
ClassLoader oldClassLoader = Thread.currentThread().getContextClassLoader();
Thread.currentThread().setContextClassLoader(NO_UNSAFE);
try {
runTestMethod(NO_VAR_HANDLE);
} finally {
Thread.currentThread().setContextClassLoader(oldClassLoader);
}

Thread.currentThread().setContextClassLoader(NO_UNSAFE);
try {
runTestMethod(NO_UNSAFE);
Expand Down Expand Up @@ -142,4 +162,8 @@ public Class<?> loadClass(String name) throws ClassNotFoundException {
}
};
}

private static boolean isJava8() {
return JAVA_SPECIFICATION_VERSION.value().equals("1.8");
}
}
172 changes: 149 additions & 23 deletions guava/src/com/google/common/util/concurrent/AbstractFuture.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,11 @@
import com.google.common.util.concurrent.internal.InternalFutures;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.annotations.ForOverride;
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.AccessController;
import java.security.PrivilegedActionException;
Expand Down Expand Up @@ -152,35 +155,60 @@ public final boolean cancel(boolean mayInterruptIfRunning) {

private static final AtomicHelper ATOMIC_HELPER;

/**
* Returns the result of calling {@link MethodHandles#lookup} from inside {@link AbstractFuture}.
* By virtue of being created there, it has access to the private fields of {@link
* AbstractFuture}, so that access is available to anyone who calls this method—specifically, to
* {@link VarHandleAtomicHelper}.
*
* <p>This "shouldn't" be necessary: {@link VarHandleAtomicHelper} and {@link AbstractFuture}
* "should" be nestmates, so a call to {@link MethodHandles#lookup} inside {@link
* VarHandleAtomicHelper} "should" have access to each other's private fields. However, our
* open-source build uses {@code -source 8 -target 8}, so the class files from that build can't
* express nestmates. Thus, when those class files are used from Java 9 or higher (i.e., high
* enough to trigger the {@link VarHandle} code path), such a lookup would fail with an {@link
* IllegalAccessException}.
*
* <p>Note that we do not have a similar problem with the fields in {@link Waiter} because those
* fields are not private. (We could solve the problem with {@link AbstractFuture} fields in the
* same way if we wanted.)
*/
private static MethodHandles.Lookup methodHandlesLookupFromWithinAbstractFuture() {
return MethodHandles.lookup();
}

static {
AtomicHelper helper;
Throwable thrownUnsafeFailure = null;
Throwable thrownAtomicReferenceFieldUpdaterFailure = null;

try {
helper = new UnsafeAtomicHelper();
} catch (Exception | Error unsafeFailure) { // sneaky checked exception
thrownUnsafeFailure = unsafeFailure;
// catch absolutely everything and fall through to our
// 'AtomicReferenceFieldUpdaterAtomicHelper' The access control checks that ARFU does means
// the caller class has to be AbstractFuture instead of
// AtomicReferenceFieldUpdaterAtomicHelper, so we annoyingly define these here
helper = VarHandleAtomicHelperMaker.INSTANCE.tryMakeVarHandleAtomicHelper();
if (helper == null) {
try {
helper =
new AtomicReferenceFieldUpdaterAtomicHelper(
newUpdater(Waiter.class, Thread.class, "thread"),
newUpdater(Waiter.class, Waiter.class, "next"),
newUpdater(AbstractFuture.class, Waiter.class, "waiters"),
newUpdater(AbstractFuture.class, Listener.class, "listeners"),
newUpdater(AbstractFuture.class, Object.class, "value"));
} catch (Exception // sneaky checked exception
| Error atomicReferenceFieldUpdaterFailure) {
// Some Android 5.0.x Samsung devices have bugs in JDK reflection APIs that cause
// getDeclaredField to throw a NoSuchFieldException when the field is definitely there.
// For these users fallback to a suboptimal implementation, based on synchronized. This will
// be a definite performance hit to those users.
thrownAtomicReferenceFieldUpdaterFailure = atomicReferenceFieldUpdaterFailure;
helper = new SynchronizedHelper();
helper = new UnsafeAtomicHelper();
} catch (Exception | Error unsafeFailure) { // sneaky checked exception
thrownUnsafeFailure = unsafeFailure;
// catch absolutely everything and fall through to our
// 'AtomicReferenceFieldUpdaterAtomicHelper' The access control checks that ARFU does means
// the caller class has to be AbstractFuture instead of
// AtomicReferenceFieldUpdaterAtomicHelper, so we annoyingly define these here
try {
helper =
new AtomicReferenceFieldUpdaterAtomicHelper(
newUpdater(Waiter.class, Thread.class, "thread"),
newUpdater(Waiter.class, Waiter.class, "next"),
newUpdater(AbstractFuture.class, Waiter.class, "waiters"),
newUpdater(AbstractFuture.class, Listener.class, "listeners"),
newUpdater(AbstractFuture.class, Object.class, "value"));
} catch (Exception // sneaky checked exception
| Error atomicReferenceFieldUpdaterFailure) {
// Some Android 5.0.x Samsung devices have bugs in JDK reflection APIs that cause
// getDeclaredField to throw a NoSuchFieldException when the field is definitely there.
// For these users fallback to a suboptimal implementation, based on synchronized. This
// will be a definite performance hit to those users.
thrownAtomicReferenceFieldUpdaterFailure = atomicReferenceFieldUpdaterFailure;
helper = new SynchronizedHelper();
}
}
}
ATOMIC_HELPER = helper;
Expand All @@ -202,6 +230,42 @@ public final boolean cancel(boolean mayInterruptIfRunning) {
}
}

private enum VarHandleAtomicHelperMaker {
INSTANCE {
/**
* Implementation used by non-J2ObjC environments (aside, of course, from those that have
* supersource for the entirety of {@link AbstractFuture}).
*/
@Override
@J2ObjCIncompatible
@CheckForNull
AtomicHelper tryMakeVarHandleAtomicHelper() {
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), 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");
return new VarHandleAtomicHelper();
} catch (ClassNotFoundException beforeJava9) {
return null;
}
}
};

/** Implementation used by J2ObjC environments, overridden for other environments. */
@CheckForNull
AtomicHelper tryMakeVarHandleAtomicHelper() {
return null;
}
}

/** Waiter links form a Treiber stack, in the {@link #waiters} field. */
private static final class Waiter {
static final Waiter TOMBSTONE = new Waiter(false /* ignored param */);
Expand Down Expand Up @@ -1344,6 +1408,68 @@ abstract boolean casListeners(
abstract boolean casValue(AbstractFuture<?> future, @CheckForNull Object expect, Object update);
}

/** {@link AtomicHelper} based on {@link VarHandle}. */
@J2ObjCIncompatible
// We use this class only after confirming that VarHandle is available at runtime.
@SuppressWarnings("Java8ApiChecker")
@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 = methodHandlesLookupFromWithinAbstractFuture();
try {
waiterThreadUpdater = lookup.findVarHandle(Waiter.class, "thread", Thread.class);
waiterNextUpdater = lookup.findVarHandle(Waiter.class, "next", Waiter.class);
waitersUpdater = lookup.findVarHandle(AbstractFuture.class, "waiters", Waiter.class);
listenersUpdater = lookup.findVarHandle(AbstractFuture.class, "listeners", Listener.class);
valueUpdater = lookup.findVarHandle(AbstractFuture.class, "value", Object.class);
} catch (ReflectiveOperationException e) {
throw new ExceptionInInitializerError(e);
}
}

@Override
void putThread(Waiter waiter, Thread newValue) {
waiterThreadUpdater.setRelease(waiter, newValue);
}

@Override
void putNext(Waiter waiter, @CheckForNull Waiter newValue) {
waiterNextUpdater.setRelease(waiter, newValue);
}

@Override
boolean casWaiters(
AbstractFuture<?> future, @CheckForNull Waiter expect, @CheckForNull Waiter update) {
return waitersUpdater.compareAndSet(future, expect, update);
}

@Override
boolean casListeners(AbstractFuture<?> future, @CheckForNull Listener expect, Listener update) {
return listenersUpdater.compareAndSet(future, expect, update);
}

@Override
Listener gasListeners(AbstractFuture<?> future, Listener update) {
return (Listener) listenersUpdater.getAndSet(future, update);
}

@Override
Waiter gasWaiters(AbstractFuture<?> future, Waiter update) {
return (Waiter) waitersUpdater.getAndSet(future, update);
}

@Override
boolean casValue(AbstractFuture<?> future, @CheckForNull Object expect, Object update) {
return valueUpdater.compareAndSet(future, expect, update);
}
}

/**
* {@link AtomicHelper} based on {@link sun.misc.Unsafe}.
*
Expand Down

0 comments on commit 06f6188

Please sign in to comment.