GeezerVison is showing github.com — Replace Unsafe and illegal reflection operations · Issue #42 · lmdbjava/lmdbjava
Replace Unsafe and illegal reflection operations #42DescriptionGave 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
Reacted by Dunemaster, Roman K, bademux, 9rb, Eric Dallo and Daniel Veihelmann Activitybp-alex commented on Jan 11, 2017 Thanks @phraktle. Are there any changes you'd suggest to improve Java 9 readiness? phraktle commented on Jan 12, 2017 ContributorAuthor More actions 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 commented on Feb 6, 2017 Is there going to be any replacement for the sort of Unsafe ByteBuffer manipulations done here? VarHandles perhaps? bp-alex commented on Feb 6, 2017 Any PRs that improve our Java 9 support are most welcome. Unfortunately I have limited time to work on this at the moment. bp-alex commented on Mar 17, 2017 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 commented on Mar 19, 2017 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 commented on Mar 20, 2017 krisskross commented on Mar 20, 2017 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/ bp-alex commented on Mar 21, 2017 As LWJGL notes, VarHandles do not allow code to arbitrarily access another module's private fields.
I suggest we:
- 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.
- 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.
- 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).
- 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) bp-alex commented on Mar 21, 2017 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 commented on Mar 22, 2017 ContributorAuthor More actions 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 commented on Mar 22, 2017 ContributorAuthor More actions krisskross commented on Mar 22, 2017 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. bp-alex commented on Mar 25, 2017
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). Reacted by Christofer Sjögren, Steve Hu and Vladislav P DigitalSmile commented on Sep 27, 2017 Java 9 has been released couple of days ago, so what's the deal? 9rb commented on Oct 29, 2020 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 commented on Jun 3, 2021 Since JDK 16 strong JDK internals encapsulation started and will continue with JDK17.
This also affects upstream libraries like datalevin/datalevin#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 commented on Jul 5, 2021 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 commented on Jul 15, 2021 @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 commented on Aug 15, 2021 With each new java version, the migration is looking more and more necessary. bp-alex commented on Aug 17, 2021 Commit 5687bd6 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. Reacted by Viktor Szathmáry, at055612, Huahai Yang, Felix Ng, Bertil Chapuis, Jacky Nguyen, Devon Stewart, Pieter Martin, Daniel Veihelmann and Davide Angelocola ieugen commented on Aug 17, 2021 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. bp-alex commented on Jun 11, 2022 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 commented on Jun 11, 2022 @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. bp-alex commented on Jun 15, 2022
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). jimfcarroll commented on Dec 6, 2024 tran4o commented on Mar 19, 2025 bp-alex commented on Oct 24, 2025 Closed in response to #259 which provides a concrete pathway forward.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|