Skip to content
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

Need JDK17 Compatable JAR #6806

Open
koundinyarvs opened this issue Oct 25, 2023 · 10 comments
Open

Need JDK17 Compatable JAR #6806

koundinyarvs opened this issue Oct 25, 2023 · 10 comments

Comments

@koundinyarvs
Copy link

The following classes are using sun.misc.Unsafe

com.google.common.cache.Striped64
com.google.common.hash.LittleEndianByteArray
com.google.common.primitives.UnsignedBytes
com.google.common.util.concurrent.AbstractFuture

class sun.misc.Unsafe is a jdk internal class. package sun.misc is jdk internal after jdk 8. In jdk16, --illegal-access=deny was default. This option is ignored in jdk 17. usage context: run method declaration (return type) 

Refer http://openjdk.java.net/jeps/260

@cpovirk
Copy link
Member

cpovirk commented Oct 25, 2023

Can you provide some information about what problem you're seeing? We have included fallback implementations for cases in which Unsafe is not available—except, it appears, perhaps not for Striped64. I would expect for those normally to automatically kick in. Are you using some kind of static analyzer that flags the Unsafe usages?

@koundinyarvs
Copy link
Author

Hi @cpovirk

Yes we are using a static analyzer to see what are the packages that java 17 doesn't like to expose usage of CLASS sun.misc.Unsafe and it is identifying
Striped64
LittleEndianByteArray
UnsignedBytes
AbstractFuture
As some classes in guava which are using sun.misc.Unsafe and this needs to be avoided
The analyzer is complaining that

class sun.misc.Unsafe is a jdk internal class. package sun.misc is jdk internal after jdk 8. In jdk16, --illegal-access=deny was default. This option is ignored in jdk 17. usage context: sun.misc.Unsafe.getUnsafe call

@cpovirk
Copy link
Member

cpovirk commented Oct 28, 2023

Is there something that we can put on our code to suppress the static analyzer for the classes that have a fallback implementation? If we can do that without adding any dependencies, then we'd probably do it.

We probably should look at Striped64. (Note that it's part of common.cache, and as always, we encourage people to use Caffeine for caching nowadays.)

@cpovirk
Copy link
Member

cpovirk commented Nov 21, 2023

On the Striped64 front:

The ViewCVS frontend for the jsr166 repo has been offline for a while now, I believe. But the command-line instructions still work. The repo includes a copy of Striped64 that uses MethodHandle+VarHandle, so we could use that if we don't want to figure things out ourselves.

Or perhaps we just want to use LongAdder? As noted, that might cause trouble for GWT/J2CL, but we could use supersource. As also hinted there, it would cause trouble for Android, too, if it were the only approach (since the class isn't available there until API Level 24). But it would be fine with a fallback to Unsafe.

@marcphilipp
Copy link

JDK 24-ea+26 or later now issues this warning by default (see MNG-8399 for all of them):

WARNING: A terminally deprecated method in sun.misc.Unsafe has been called
WARNING: sun.misc.Unsafe::objectFieldOffset has been called by com.google.common.util.concurrent.AbstractFuture$UnsafeAtomicHelper (file:/home/marc/.asdf/installs/maven/3.9.9/lib/guava-33.2.1-jre.jar)
WARNING: Please consider reporting this to the maintainers of class com.google.common.util.concurrent.AbstractFuture$UnsafeAtomicHelper
WARNING: sun.misc.Unsafe::objectFieldOffset will be removed in a future release 

For now, it can be suppressed using --sun-misc-unsafe-memory-access=allow but according to JEP 498 that's going to change in "JDK 26 or later".

@cpovirk
Copy link
Member

cpovirk commented Dec 4, 2024

Thanks. Our current JDK@head project is SecurityManager, which you've probably seen produce similar warnings :) I imagine that this one will be in our sights soon. We'll compare performance of the various AtomicHelper implementations under various JDKs and then see if we should reorder them, introduce a new one (VarHandle-based?), or what.

@cpovirk
Copy link
Member

cpovirk commented Dec 4, 2024

We have an existing benchmark (in Caliper, sadly, rather than JMH) for single-threaded use of AbstractFuture. The difference between the existing Unsafe implementation and the existing AtomicReferenceFieldUpdater version is mostly hard to detect in the noise, but Unsafe has a noticeable advantage for a benchmark that merely repeatedly creates, sets, and gets. (Both implementations are at least clearly better than the fallback synchronized-based implementation.)

The only immediate takeaway from that is that we can't just flip our default to the AtomicReferenceFieldUpdater implementation today. But we can still eventually see about using VarHandle, and we can see about making our AtomicReferenceFieldUpdater-typed fields static, which I believe should allow for better optimization.

@cpovirk
Copy link
Member

cpovirk commented Dec 4, 2024

static does appear to eliminate the performance difference in the simple test. Further benchmarking is of course needed. [edit: I wonder if I messed this up? Comments in the source suggest that I should have seen access problems that forced fallback to the synchronized implementation, but then I didn't see results that slow....]

Another thing for us to remember is that the way we try to initialize the various helers with try-catch is probably wrong. I would hope that defaulting to our AtomicReferenceFieldUpdater implementation would actually be the safest move there, since that implementation should never fail to load (aside from the old Samsung bug—though perhaps it could make things worse there?).

@cpovirk
Copy link
Member

cpovirk commented Dec 4, 2024

Initial benchmarks for VarHandle also look promising. However, they do make me worry more about the try-catch approach, since an attempt to use VarHandle under Java 8 would fail. We could probably still do it if we guard it with reflection to look at whether VarHandle exists.

@ben-manes
Copy link
Contributor

ben-manes commented Dec 4, 2024

AtomicReferenceFieldUpdater became much faster in OpenJDK's lead up to VarHandles, whereas in Java 5 it was slow due to reflection and allocations at every invocation, iirc. If that helps here is Shiplev's discussion on it using JHM:
Faster Atomic*FieldUpdaters for Everyone

copybara-service bot pushed a commit that referenced this issue Dec 12, 2024
(followup cl/705490132)

The method requires JDK 9+ to build, but our Maven build [always builds with JDK 23](#6549 (comment)). It also requires JDK 9+ to _run_, so I've added code to skip running the test under older versions.

Much of my goal here is to shake out any ways that we might cause problems if we begin conditionally using Java 9+ APIs more widely, such as [for `VarHandle`](#6806 (comment)).

RELNOTES=n/a
PiperOrigin-RevId: 705498400
copybara-service bot pushed a commit that referenced this issue Dec 12, 2024
(followup cl/705490132)

The method requires JDK 9+ to build, but our Maven build [always builds with JDK 23](#6549 (comment)). It also requires JDK 9+ to _run_, so I've added code to skip running the test under older versions.

Much of my goal here is to shake out any ways that we might cause problems if we begin conditionally using Java 9+ APIs more widely, such as [for `VarHandle`](#6806 (comment)).

RELNOTES=n/a
PiperOrigin-RevId: 705498400
copybara-service bot pushed a commit that referenced this issue Dec 12, 2024
(followup cl/705490132)

The method requires JDK 9+ to build, but our Maven build [always builds with JDK 23](#6549 (comment)). It also requires JDK 9+ to _run_, so I've added code to skip running the test under older versions.

Much of my goal here is to shake out any ways that we might cause problems if we begin conditionally using Java 9+ APIs more widely, such as [for `VarHandle`](#6806 (comment)).

RELNOTES=n/a
PiperOrigin-RevId: 705498400
copybara-service bot pushed a commit that referenced this issue Dec 12, 2024
(followup cl/705490132)

The method requires JDK 9+ to build, but our Maven build [always builds with JDK 23](#6549 (comment)). It also requires JDK 9+ to _run_, so I've added code to skip running the test under older versions.

Much of my goal here is to shake out any ways that we might cause problems if we begin conditionally using Java 9+ APIs more widely, such as [for `VarHandle`](#6806 (comment)).

RELNOTES=n/a
PiperOrigin-RevId: 705512728
copybara-service bot pushed a commit that referenced this issue Dec 13, 2024
See #6806 (comment).

Changes:

- "`SafeAtomicHelper`" is arguably already too generic a name for that class, given that we have a `SynchronizedAtomicHelper` that also avoids using `Unsafe`. It's going to become even more overly generic (and more overly scary) when we likely introduce a `VarHandle`-based alternative. (And maybe we'll even remove the `Unsafe`-based one entirely?) Rename it.
- Remove Javadoc from implementation classes, since it merely duplicates that from the superclass.
- Fix links in the (package-private) Javadoc.

I considered also renaming the `AtomicHelper` methods to match the terminology of `VarHandle`. This would mean only renaming `putThread`+`putNext` to... `setReleaseThread`? `setThreadReleasedly`? `setThreadUsingReleaseAccessMode`? I didn't find anything that I particularly liked.

RELNOTES=n/a
PiperOrigin-RevId: 705587524
copybara-service bot pushed a commit that referenced this issue Dec 13, 2024
See #6806 (comment).

Changes:

- "`SafeAtomicHelper`" is arguably already too generic a name for that class, given that we have a `SynchronizedAtomicHelper` that also avoids using `Unsafe`. It's going to become even more overly generic (and more overly scary) when we likely introduce a `VarHandle`-based alternative. (And maybe we'll even remove the `Unsafe`-based one entirely?) Rename it.
- Remove Javadoc from implementation classes, since it merely duplicates that from the superclass.
- Fix links in the (package-private) Javadoc.

I considered also renaming the `AtomicHelper` methods to match the terminology of `VarHandle`. This would mean only renaming `putThread`+`putNext` to... `setReleaseThread`? `setThreadReleasedly`? `setThreadUsingReleaseAccessMode`? I didn't find anything that I particularly liked.

RELNOTES=n/a
PiperOrigin-RevId: 705868797
copybara-service bot pushed a commit that referenced this issue Dec 17, 2024
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
copybara-service bot pushed a commit that referenced this issue Dec 18, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants
@marcphilipp @ben-manes @cpovirk @chaoren @koundinyarvs and others