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

Dependence on sun.misc.Unsafe getting troublesome now. #273

Closed
StrangeNoises opened this issue Sep 26, 2018 · 23 comments
Closed

Dependence on sun.misc.Unsafe getting troublesome now. #273

StrangeNoises opened this issue Sep 26, 2018 · 23 comments

Comments

@StrangeNoises
Copy link

In #122 you said when Java 9 was out you'd probably do a version 3.0 using VarHandles instead of depending on the Unsafe stuff. Now Java11 is out and I'm having to jlink my server jre with jdk.unsupported just so caffeine can work. (tbh I'm not entirely sure why I didn't have to do this in Java10.) Any sign of that 3.0 version using VarHandles? :-)

@ben-manes
Copy link
Owner

Sorry I've been swamped and falling really far behind. I usually work on this project during my weekends, but I haven't had any time or been too exhausted. So lots of things need to be taken care of.

It's odd that they require that now, given that Unsafe still works fine. Maybe it to prepare one LTS before the 2021 LTS that will likely remove Unsafe for good?

For v3, I would like to rewrite the JCache implementation to use the native per-entry expiration support, since that wasn't implemented when the adapter was written. Instead it has to use an uglier approach, but also used by the spec leads, of relying on maximum size with dead entries. Since this would change the configuration format, I want to make all of the breaking changes together. It might be better to update the config format and punt on the rewrite work, though, given my struggles to keep up with things.

For VarHandles, my only concern is how to port StripedBuffer, which uses the thread field (as forked from Java's Striped64). I think the VarHandle should peek into it, but not positive. So likely it should be updated with a fallback chain (e.g. to threadlocal field) just in case.

@ahasani
Copy link

ahasani commented Nov 10, 2018

Like other cache/map implementations using sun.misc.Unsafe we can use --add-exports java.base/sun.nio.ch=ALL-UNNAMED jvm option for now

@re-thc
Copy link

re-thc commented Feb 11, 2019

I'm not so sure that VarHandles has better performance than Unsafe. Static VarHandles maybe but that's not really suitable for the use case. Just to note that in Java 10 there's also jdk.internal.misc.Unsafe which has *unaligned access methods which may enable even more performance (but it's hidden and restricted with required --add-exports / --add-opens). sun.misc.Unsafe is a wrapper around the internal version that lacks a few methods.

@sunnythepooh
Copy link

Have we considered using multi-version jar? (implemented in Java 9)

https://openjdk.java.net/jeps/238

@oobles
Copy link

oobles commented Sep 13, 2019

Just hit this issue moving a project to Java 11.

@ben-manes
Copy link
Owner

This shouldn't impact moving to Java 11, as I'm using that without any special flags. What issue did you run into?

There doesn't seem to be a rush by the JDK team to remove Unsafe and JCTools still prefers it for performance reasons. Since it requires a major release and hasn't seemed pressing, I've been working small scoped improvements instead that are more amenable to weekend / commute time.

@oobles
Copy link

oobles commented Sep 20, 2019

I've been moving to use the module system and must admit my ignorance. I thought Unsafe was gone in Java 11. Took me a while to find the VM argument --add-modules=jdk.unsupported to get access to Unsafe again. Adding here for other people who hit this issue. :) Thanks for indirectly sending me in the right direction.

@sigram
Copy link

sigram commented Nov 21, 2019

This was brought to me recently by one of our customers, now that Solr uses Caffeine - their security team flagged this as a concern. In general, I think commercial users are wary (usually for good reasons) of adding any flags with "unsupported" in their names...

@ben-manes
Copy link
Owner

I believe you only need --add-modules=jdk.unsupported if you are using the module system instead of the classpath, e.g. for native images. I have not had any problems in Java 9-12 projects without this flag in classpath-based projects. I haven't tried 13-14, though, and vast majority of projects are classpath based so it generally shouldn't be a concern.

@michaelhixson
Copy link

I believe you only need --add-modules=jdk.unsupported if you are using the module system instead of the classpath, e.g. for native images.

Well, I'm using jlink with --add-modules to produce a smaller Java runtime, but my application is still using the classpath and is not modular.

I'm not sure how common this is or how good of an idea it is. I only started doing this recently with one application.

(Since my application and dependencies are not modular, coming up with the list of modules for jlink was a process of trial and error. I would start the application and look for NoClassDefFoundErrors, find out which modules those classes belonged to, add them to jlink, and try again.)

@ben-manes
Copy link
Owner

Oh thanks. I didn't realize jlink also works with the classpath and had assumed it required opting in all of the way. Moving to a custom runtime jvm does simplify deployments for newer projects, so I'm sure it will become more popular.

It would probably make more sense to gather a list of changes for 3.0, implement any API facing, and bump the base JDK requirements. Then removing Unsafe or other enhancements can be done incrementally rather than needing a big bang release.

@GedMarc
Copy link

GedMarc commented Dec 11, 2019

13 looks a bit more blocked -

javax.persistence.PersistenceException: [PersistenceUnit: ActivityMasterUT] Unable to build Hibernate SessionFactory
	at [email protected]_0-SNAPSHOT/org.hibernate.jpa.boot.internal.EntityManagerFactoryBuilderImpl.persistenceException(EntityManagerFactoryBuilderImpl.java:1314)
	at [email protected]_0-SNAPSHOT/org.hibernate.jpa.boot.internal.EntityManagerFactoryBuilderImpl.build(EntityManagerFactoryBuilderImpl.java:1240)
	at [email protected]_0-SNAPSHOT/org.hibernate.jpa.HibernatePersistenceProvider.createEntityManagerFactory(HibernatePersistenceProvider.java:56)
	at [email protected]_0-SNAPSHOT/javax.persistence.Persistence.createEntityManagerFactory(Persistence.java:80)
	at [email protected]_0-SNAPSHOT/com.guicedee.guicedpersistence.injectors.CustomJpaPersistService.start(CustomJpaPersistService.java:160)
	at [email protected]_0-SNAPSHOT/com.guicedee.guicedpersistence.db.DbStartup.postLoad(DbStartup.java:76)
	at [email protected]_0-SNAPSHOT/com.guicedee.guicedpersistence.db.DbStartup.run(DbStartup.java:100)
	at java.base/java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1425)
	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
	at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1016)
	at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1665)
	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1598)
	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:177)
Caused by: org.hibernate.MappingException: Could not get constructor for org.hibernate.persister.entity.SingleTableEntityPersister
	at [email protected]_0-SNAPSHOT/org.hibernate.persister.internal.PersisterFactoryImpl.createEntityPersister(PersisterFactoryImpl.java:123)
	at [email protected]_0-SNAPSHOT/org.hibernate.persister.internal.PersisterFactoryImpl.createEntityPersister(PersisterFactoryImpl.java:77)
	at [email protected]_0-SNAPSHOT/org.hibernate.metamodel.internal.MetamodelImpl.initialize(MetamodelImpl.java:181)
	at [email protected]_0-SNAPSHOT/org.hibernate.internal.SessionFactoryImpl.<init>(SessionFactoryImpl.java:305)
	at [email protected]_0-SNAPSHOT/org.hibernate.boot.internal.SessionFactoryBuilderImpl.build(SessionFactoryBuilderImpl.java:462)
	at [email protected]_0-SNAPSHOT/org.hibernate.jpa.boot.internal.EntityManagerFactoryBuilderImpl.build(EntityManagerFactoryBuilderImpl.java:1237)
	... 11 more
Caused by: org.hibernate.HibernateException: Unable to instantiate default tuplizer [org.hibernate.tuple.entity.PojoEntityTuplizer]
	at [email protected]_0-SNAPSHOT/org.hibernate.tuple.entity.EntityTuplizerFactory.constructTuplizer(EntityTuplizerFactory.java:91)
	at [email protected]_0-SNAPSHOT/org.hibernate.tuple.entity.EntityTuplizerFactory.constructDefaultTuplizer(EntityTuplizerFactory.java:116)
	at [email protected]_0-SNAPSHOT/org.hibernate.tuple.entity.EntityMetamodel.<init>(EntityMetamodel.java:413)
	at [email protected]_0-SNAPSHOT/org.hibernate.persister.entity.AbstractEntityPersister.<init>(AbstractEntityPersister.java:589)
	at [email protected]_0-SNAPSHOT/org.hibernate.persister.entity.SingleTableEntityPersister.<init>(SingleTableEntityPersister.java:125)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:481)
	at [email protected]_0-SNAPSHOT/org.hibernate.persister.internal.PersisterFactoryImpl.createEntityPersister(PersisterFactoryImpl.java:96)
	... 16 more
Caused by: java.lang.reflect.InvocationTargetException
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:481)
	at [email protected]_0-SNAPSHOT/org.hibernate.tuple.entity.EntityTuplizerFactory.constructTuplizer(EntityTuplizerFactory.java:88)
	... 26 more
Caused by: java.lang.IllegalArgumentException: Could not create type
	at net.bytebuddy/net.bytebuddy.TypeCache.findOrInsert(TypeCache.java:154)
	at net.bytebuddy/net.bytebuddy.TypeCache$WithInlineExpunction.findOrInsert(TypeCache.java:365)
	at net.bytebuddy/net.bytebuddy.TypeCache.findOrInsert(TypeCache.java:174)
	at net.bytebuddy/net.bytebuddy.TypeCache$WithInlineExpunction.findOrInsert(TypeCache.java:376)
	at [email protected]_0-SNAPSHOT/org.hibernate.bytecode.internal.bytebuddy.ByteBuddyState.load(ByteBuddyState.java:175)
	at [email protected]_0-SNAPSHOT/org.hibernate.bytecode.internal.bytebuddy.ByteBuddyState.loadProxy(ByteBuddyState.java:99)
	at [email protected]_0-SNAPSHOT/org.hibernate.proxy.pojo.bytebuddy.ByteBuddyProxyHelper.buildProxy(ByteBuddyProxyHelper.java:54)
	at [email protected]_0-SNAPSHOT/org.hibernate.proxy.pojo.bytebuddy.ByteBuddyProxyFactory.postInstantiate(ByteBuddyProxyFactory.java:61)
	at [email protected]_0-SNAPSHOT/org.hibernate.tuple.entity.PojoEntityTuplizer.buildProxyFactory(PojoEntityTuplizer.java:163)
	at [email protected]_0-SNAPSHOT/org.hibernate.tuple.entity.AbstractEntityTuplizer.<init>(AbstractEntityTuplizer.java:155)
	at [email protected]_0-SNAPSHOT/org.hibernate.tuple.entity.PojoEntityTuplizer.<init>(PojoEntityTuplizer.java:59)
	... 32 more
Caused by: java.lang.UnsupportedOperationException: Could not access Unsafe class: sun.misc.Unsafe.defineClass(java.lang.String,[B,int,int,java.lang.ClassLoader,java.security.ProtectionDomain)
	at net.bytebuddy/net.bytebuddy.dynamic.loading.ClassInjector$UsingUnsafe$Dispatcher$Unavailable.initialize(ClassInjector.java:2031)
	at net.bytebuddy/net.bytebuddy.dynamic.loading.ClassInjector$UsingUnsafe.injectRaw(ClassInjector.java:1764)
	at net.bytebuddy/net.bytebuddy.dynamic.loading.ClassInjector$AbstractBase.inject(ClassInjector.java:112)
	at net.bytebuddy/net.bytebuddy.dynamic.loading.ClassLoadingStrategy$ForUnsafeInjection.load(ClassLoadingStrategy.java:546)
	at net.bytebuddy/net.bytebuddy.dynamic.TypeResolutionStrategy$Passive.initialize(TypeResolutionStrategy.java:100)
	at net.bytebuddy/net.bytebuddy.dynamic.DynamicType$Default$Unloaded.load(DynamicType.java:5662)
	at [email protected]_0-SNAPSHOT/org.hibernate.bytecode.internal.bytebuddy.ByteBuddyState.lambda$load$0(ByteBuddyState.java:179)
	at net.bytebuddy/net.bytebuddy.TypeCache.findOrInsert(TypeCache.java:152)
	... 42 more

byte buddy 1.10

@ben-manes
Copy link
Owner

Is that related though? The stacktrace doesn't include this library and we do not dynamically generate classes (Unsafe.defineClass). Our usage is for memory barriers for load/store (e.g. volatile vs non-volatile reads). ByteBuddy doesn't use our library for its TypeCache. So off-hand I don't think it our problem (yet!).

@ben-manes
Copy link
Owner

I have the initial start on this in the v3 branch.

The main difficulty is StripedBuffer, which is a fork of Java's Striped64 used by LongAdder. This uses a thread local probe to choose an array slot to work with. If contention is detected, it is updated to a new random value. This allows for dynamically spreading threads across an array to update their location. For Java this is used to increment counter cells and we use it for ring buffers.

Java reuses the ThreadLocalRandom probe value stashed in Thread as threadLocalRandomProbe. We can use this cheaply by either Unsafe or a VarHandle for existing JDKs, just like j.u.c. does. When these methods are disallowed then we have to fallback to a ThreadLocal which is less efficient (a hashmap lookup). Perhaps later with Loom we'll get a more optimal concept, since ThreadLocal won't make much sense here due to virtual threads.

The other major case to review our usage of JCTools which hasn't migrated due to Unsafe being much faster in current JDKs. Similarly we'll probably use Unsafe and a fallback to VarHandles if unavailable.

No timeline for v3 but those are the caveats with Unsafe.

@ben-manes
Copy link
Owner

Sorry for being slow here. In my first reply, I mentioned a possibly backwards incompatible change to the JCache module. That isn't used much, but I had wanted to improve its implementation and that seemed to require a lot of effort to redo. I took advantage of the holidays being extra quiet this year, got far with a rewrite before discovering a blocker, and took that learning to make a surgical change that resolved my concerns. So that problem is done and has no incompatibilities to worry about.

The other issues were resolved in that v3 branch. That work needs to be resurrected, but hopefully I can start making progress again. I believe all remaining tasks are small and doable as I find the time. JCTools is likely the only blocker and our use is limited, so hopefully converting code that isn't too difficult (they rightfully prefer Unsafe for performance reasons, but our case is less sensitive so its okay).

ben-manes added a commit that referenced this issue Jan 12, 2021
By default Unsafe will be used if available, due to the performance
benefits as discussed in [1]. If not available then access will
fallback to VarHandles. The queue sanity tests run against both
variants.

[1] JCTools/JCTools#157
ben-manes added a commit that referenced this issue Jan 12, 2021
By default Unsafe will be used if available, due to the performance
benefits as discussed in [1]. If not available then access will
fallback to VarHandles. The queue sanity tests run against both
variants.

[1] JCTools/JCTools#157
ben-manes added a commit that referenced this issue Jan 17, 2021
By default Unsafe will be used if available, due to the performance
benefits as discussed in [1]. If not available then access will
fallback to VarHandles. The queue sanity tests run against both
variants.

[1] JCTools/JCTools#157
ben-manes added a commit that referenced this issue Feb 8, 2021
By default Unsafe will be used if available, due to the performance
benefits as discussed in [1]. If not available then access will
fallback to VarHandles. The queue sanity tests run against both
variants.

[1] JCTools/JCTools#157
ben-manes added a commit that referenced this issue Feb 8, 2021
By default Unsafe will be used if available, due to the performance
benefits as discussed in [1]. If not available then access will
fallback to VarHandles. The queue sanity tests run against both
variants.

[1] JCTools/JCTools#157
ben-manes added a commit that referenced this issue Feb 15, 2021
By default Unsafe will be used if available, due to the performance
benefits as discussed in [1]. If not available then access will
fallback to VarHandles. The queue sanity tests run against both
variants.

[1] JCTools/JCTools#157
ben-manes added a commit that referenced this issue Feb 16, 2021
By default Unsafe will be used if available, due to the performance
benefits as discussed in [1]. If not available then access will
fallback to VarHandles. The queue sanity tests run against both
variants.

[1] JCTools/JCTools#157
ben-manes added a commit that referenced this issue Feb 16, 2021
By default Unsafe will be used if available, due to the performance
benefits as discussed in [1]. If not available then access will
fallback to VarHandles. The queue sanity tests run against both
variants.

[1] JCTools/JCTools#157
@ben-manes
Copy link
Owner

A 3.0-SNAPSHOT is now available if anyone wants to test disallowing Unsafe as a module dependency. Where appropriate for best performance, Unsafe is used If available and otherwise it fallbacks to VarHandles.

This branch is ready for release, so I plan on letting it bake for a week in case anything else comes up before making an official cut.

@ben-manes
Copy link
Owner

Released in 3.0

@ben-manes
Copy link
Owner

This was brought to me recently by one of our customers, now that Solr uses Caffeine - their security team flagged this as a concern. In general, I think commercial users are wary (usually for good reasons) of adding any flags with "unsupported" in their names...

@sigram You might want to upgrade Solr to v3 to resolve your concerns. I notice that project is JDK11 on master, so this should be fine. There are not incompatible changes that should affect you. The new release will use Unsafe if available, but it is not required. If the flag is not included it will work fine via VarHandles, which has similar performance.

I am not sure how Solr manages version upgrades in the new Gradle build. You might consider setting up something like the gradle-update-checker Github Action, which uses my gradle-versions-plugin. Obviously you don't want to upgrade too rapidly but you might want to track that updates are available for bug fixes and such things.

@ben-manes
Copy link
Owner

Further improvements allowed for removal of Unsafe entirely, rather than using fallback strategies if unavailable. The new probing strategy for StripedBuffer performs equally well by using incremental hashing instead of the JDK's perfect hashing via threadLocalRandomProbe. As that was removed from the read buffer, the remaining usage was write buffer's. That case was already satisfied by theVarHandles fallback, but hadn't been promoted due to not being fully performance analyzed by the time of release. In the next minor version we'll no longer use sun.misc.Unsafe because it offers no benefits.

@ben-manes
Copy link
Owner

Released 3.0.2 with removal of sun.misc.Unsafe.

@bcmedeiros
Copy link

bcmedeiros commented Sep 12, 2021

@ben-manes any chances of getting this backported to 2.x? I tried to use 3.x in this PR, but as the project needs to target Java 8, there is lots of test failures.

I'm happy to help if that's something you're interested in and you give me some direction.

@ben-manes
Copy link
Owner

Sorry, but this won't be backported. We use Unsafe for slimmer entries (no Atomic wrappers), which is now possible using VarHandles. The only alternative is to use AtomicReferenceFieldUpdater, but this uses reflection making it generally disfavored. The code generator and various other direct usages would have to migrate to either approach and I don't think it would be preferable for slow or memory bloated atomic operations. The other usages of Unsafe were replaced by algorithmic changes. It seems wiser to upgrade JDKs, e.g. Java 8 is EOL in six months (March 2022).

@bcmedeiros
Copy link

Fair enough, thanks for the reply!
I personally don't use Java 8 for a long time, I agree with you, but the library I'm contributing to supports Java 8, so it's beyond my decision.

KengoTODA added a commit to KengoTODA/javadocky that referenced this issue Oct 19, 2021
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

No branches or pull requests

10 participants