Replace Unsafe and illegal reflection operations

[phraktle]

Gave a quick try to lmdbjava with JDK 9 and wanted to record the findings here.

Due the the class access restrictions imposed by Jigsaw, the following JVM arguments have to be specified:

--add-opens java.base/java.nio=ALL-UNNAMED --add-opens java.base/sun.nio.ch=ALL-UNNAMED

[benalexau]

Thanks @phraktle. Are there any changes you'd suggest to improve Java 9 readiness?

[phraktle]

If there was a way to avoid using Unsafe access to ByteBuffers (pushing the address manipulation to the JNI native layer), it would make it easier to adopt this library in the future. However, with the above VM args, it seems to work fine. (Also, if there was proper Jigsaw module meta-data, the use of the generic "ALL-UNNAMED" could be avoided).

[scottcarey]

Is there going to be any replacement for the sort of Unsafe ByteBuffer manipulations done here? VarHandles perhaps?

[benalexau]

Any PRs that improve our Java 9 support are most welcome. Unfortunately I have limited time to work on this at the moment.

[benalexau]

I've now had a chance to test LmdbJava with Java 9.

First, the good news:

• Some of our Unsafe use can be replaced with com.kenai.jffi.MemoryIO operations
• Netty Unpooled now allows allows wrapping a pointer-derived address
• Maven running under Java 9 can compile Java 8 while including a Java 9 module-info.java

Next, the bad news:

• Violating field encapsulation in ByteBuffer and ByteBuf is no longer permitted
• Some Maven plugins (eg dependency analysis) fail (eg ASM class errors)
• AppVeyor and Travis macOS do not yet support Java 9

The last two issues aren't major and will probably be resolved once Java 9 is GA.

The former issue is important though, as complying with ByteBuffer and ByteBuf field visibility modifiers will result in allocation and deallocation overhead on each use. On the bright side, we'd be properly using those classes and this would mean easier eventual support for Android. Plus users wouldn't be forced to use --add-opens (which apparently disappears in Java 10 anyway).

Buffers using Agrona and byte[] will continue to operate at the same speed as they presently do. This is because Agrona allows an existing buffer to inexpensively wrap a new address, and byte[] already incurs copying, allocation and deallocation costs. Two of the buffers are therefore unaffected.

How would users feel about ByteBuffer and ByteBuf being used in accordance with their API contracts? This will make them slower, but encapsulation compliance is the only major Java 9 blocker.

[krisskross]

This is all in flux IMHO. We don't know how Panama will turn out yet and moving to VarHandles seems complicated from my point of view. Its unfortunate that users will need to add extra options to run lmdbjava in Java 9, but I think patience is the right approach at this moment.

[krisskross]

I tweeted Brian Goetz and his advice is to use VarHandles to move away from Unsafe.

https://twitter.com/BrianGoetz/status/843650963370135553

[krisskross]

Some notes from the lwjgl guys.

First, lets address the elephant in the room. Yes, we do use Unsafe for this. We're not happy about it either, but there is no alternative. VarHandles in Java 9 will not help, because we're accessing private implementation details. Unless a pure Java alternative is provided in Java 9, this is the best we can do until Java 10 and Project Panama (looks like we'll stop using NIO buffers then anyway).

https://blog.lwjgl.org/memory-management-in-lwjgl-3/

[benalexau]

As LWJGL notes, VarHandles do not allow code to arbitrarily access another module's private fields.

I suggest we:

1. Remove all Unsafe use and replace with MemoryIO. That means no need to requires jdk.unsupported in the module-info.java and no more compiler warnings.
2. Make ByteBufferProxy.PROXY_SAFE allocate new instances and copy. Copying has precedent with PROXY_BA already. This means ByteBuffer is being used as its API permits.
3. Either remove ByteBufferProxy.PROXY_OPTIMAL or retain it with the current pattern of illegal reflective setAccessible(true) field access (Java 9 users must --add-opens).
4. Update the LmdbJava benchmark to quantify the current buffer alternatives.

I don't believe PROXY_OPTIMAL is worth retaining given it would occupy such a narrow niche:

Proxy	Portable (1)	Valid Use (2)	Dependencies	Flags (3)	Fast
BA	Yes	Yes	No	No	No (4, 5, 6)
SAFE	Yes	Yes	No	No	No (4, 5, 6)
OPTIMAL	No	No	No	Yes	No (6, 7)
DB	No	Yes	Yes	No	Yes
NETTY	No	Yes	Yes	No	Yes

(1) Portable across all Java-certified JVMs (2) LmdbJava correctly using the relevant buffer API (3) LmdbJava module referenced in Java 9 --add-opens flag (4) Allocation (5) Copying (6) Bounds checking (7) Reflective field access (address and length)

[benalexau]

Just noting here the Java 9 module-info.java found to work for future reference (I won't commit this given it would disrupt the build to exclude this Java file etc):

module org.lmdbjava {
   requires jdk.unsupported;
   requires agrona; 
   requires jffi;
   requires jnr.constants;
   requires jnr.ffi;
   exports org.lmdbjava;
}

[phraktle]

There's also another obvious option: implement ByteBuffer creation in JNI native code. Of course this would complicate the binding code, but it does not rely on the hacks that are being phased out in the JDK...

[phraktle]

On a related note, JDK9 introduced the --permit-illegal-access global flag, which makes it easier to trace offending code.

[krisskross]

I'm fine with the suggestion of removing OPTIMAL (unsafe and reflection) and having ByteBuffers wrap byte arrays instead.

However, i'm not sure what this solves since lmdbjava would still be transitively dependent on Unsafe through jnrffi and Agrona, which means we'll still be stuck on Java 9 unless Project Panama or something else comes along.

[benalexau]

I'm not sure what this solves since lmdbjava would still be transitively dependent on Unsafe through jnrffi and Agrona

The main thing it would solve is removing all illegal code from LmdbJava, in turn simplifying our maintenance effort, Java 9 compliance status and Android porting compatibility. Those using Agrona, Netty, safe ByteBuffer and byte[] will see no material performance difference.

I concede JNR-FFI, Agrona and Netty still have to solve these issues, but they should be better positioned to do so given they are popular, low-level libraries.

My broader (non-LmdbJava) testing with Java 9 found countless issues with other applications and libraries as well. As such I have put Java 9 on ice for now and suggest we sit back and see what happens over the next few months. This seems pragmatic anyway given the build and tooling ecosystem isn't there yet (eg Java 9 meaning broken Maven plugins, lack of working CI etc).

[DigitalSmile]

Java 9 has been released couple of days ago, so what's the deal?

[DigitalSmile]

Is this repo still being maintained?...

[krisskross]

I haven't personally felt the need to use lmdbjava as a java 9 module yet. Also haven't had time to deep dive into modules yet. But I guess the first thing to do is to settle on a module name.

[DigitalSmile]

At least it would be helpful to make the code Java 9 compatible following this guide -> https://docs.oracle.com/javase/9/migrate/toc.htm

[jubax]

I just wanted to ask if there is any progress on this issue. I'm using

--add-opens java.base/java.nio=ALL-UNNAMED --add-opens java.base/sun.nio.ch=ALL-UNNAMED where possible, but in some cases I cannot do that and then the "illegal reflective access" warnings show up. BTW: Does anybody know how to get rid of the warnings without the commandline arguments?

[julesjacobsen]

I guess you've seen this, but if not does the upcoming release of Java 15, specifically JEP-383 (Foreign-Memory Access API) help towards this?

[9rb]

We're seeing the issue reported in #167 with jdk11 WARNING: An illegal reflective access operation has occurred WARNING: Illegal reflective access by org.lmdbjava.ByteBufferProxy$AbstractByteBufferProxy (file:/var/cache/rchain-build/home-cache/coursier/v1/https/repo1.maven.org/maven2/org/lmdbjava/lmdbjava/0.8.1/lmdbjava-0.8.1.jar) to field java.nio.Buffer.address WARNING: Please consider reporting this to the maintainers of org.lmdbjava.ByteBufferProxy$AbstractByteBufferProxy WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations WARNING: All illegal access operations will be denied in a future release

[ieugen]

Since JDK 16 strong JDK internals encapsulation started and will continue with JDK17. This also affects upstream libraries like https://github.com/juji-io/datalevin/issues/47 .

The good news might be that some improvements to interact with native code are going to be adopted: https://openjdk.java.net/jeps/412 .

[jubax]

I can confirm that using

--add-opens java.base/java.nio=ALL-UNNAMED --add-opens java.base/sun.nio.ch=ALL-UNNAMED

is required when you want to use lmdbjava with Java 16. I hope this workaround still works with Java 17 (but I did not try it).

[tkorach]

@jubax According to https://openjdk.java.net/jeps/403 it should still work:

It will still be possible to use the --add-opens command-line option, or the Add-Opens JAR-file manifest attribute, to open specific packages.

However, migrating off of these packages seems like a good direction.

[maxim5]

With each new java version, the migration is looking more and more necessary.

[benalexau]

Commit 5687bd61ff8de770800fa319b4b98a0f61a92b24 shows how to add the required JVM parameters for running tests in Maven builds etc. It's implemented as a profile as we presently perform CI builds with Java 8.

https://openjdk.java.net/jeps/412 looks a promising pathway for Java 17 and above, particularly given Java 17 is LTS.

How would people feel about LmdbJava 1.x targeting Java 16 and below, with LmdbJava 2.x requiring Java 17 and above? This would draw a line in the sand and allow use of newer APIs.

[ieugen]

I think making the split like that is a very good idea. It allows the project to move forward while dropping support for old things and should keep users happy. My +1 for this.

[dtromb]

This issue is now five years old. It's hard to agree it's an "enhancement" as every application that uses lmdbjava will currently fail out of the box with a modern / relatively recent JDK.

Is there any chance of someone making an official call here so we can use lmdbjava again?

(The README says "Continuous integration testing on Linux, Windows and macOS with Java 8, 11 and 16", but 16.0.1 is not compatible due to this problem!)

[benalexau]

This issue may be 5 years old, but we've been waiting for Project Panama to release long-term, stable APIs. It was only with Java 17 (14 September 2021) did this occur.

My previous comment proposed that LmdbJava 1.x target up to Java 16 and LmdbJava 2.x target Java 17+. I still think that's the most reasonable way forward here, but I have not tried to use the new APIs myself so we may encounter unexpected issues when attempting to.

The practical issue is LmdbJava 2.x is in effect a rewrite to rely on native Java 17+ APIs. This will require a significant investment of time and unfortunately I am unable to commit to doing so in the foreseeable future. On a more positive note, we do have extensive test coverage and CI that will verify the success of any rewrite across the major operating systems with minimal work. If anyone had some time to start on this work I would welcome them to reach out, and I'll help out as I can (eg I'll prioritise native builds on some of the other platforms that have been requested, releasing an LdmbJava 1.0.0 etc).

[ieugen]

@benalexau : I think this is a sensible approach. We would all love for it to be here now but things are as they are.

I believe it would be more sensible to target JDK19 + https://openjdk.java.net/projects/jdk/19/

JDK19 has JEP 424: Foreign Function & Memory API (Preview) . https://openjdk.java.net/jeps/424 .

I believe it could be made to work with JDK17+ but it might be difficult / not worth it.

[benalexau]

I believe it would be more sensible to target JDK19 + ...... has JEP 424: FFM

I believe it could be made to work with JDK17+ but it might be difficult / not worth it.

I agree.

Quoting JEP 424:

We do not propose here to change sun.misc.Unsafe in any way. The FFM API's support for off-heap memory is an excellent alternative to the wrappers around malloc and free in sun.misc.Unsafe, namely allocateMemory, setMemory, copyMemory, and freeMemory. We hope that libraries and applications that require off-heap storage adopt the FFM API so that, in time, we can deprecate and then eventually remove these sun.misc.Unsafe methods.

Based on the above, Unsafe will not be going away any time soon. The present Unsafe opting-in and associated warnings will remain, but even hypothetically rewriting LmdbJava for Java 19+ will still at minimum require command line flags (eg to enable the FFM preview).