Skip to content

Make the guava-android copy of AbstractFuture try VarHandle when run under the JVM. #7759

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 0 commits into from

Conversation

copybara-service[bot]
Copy link
Contributor

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 (33 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 (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. 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. 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? 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0 participants