Open Ali-RS opened 2 years ago
Another idea might be to consider deprecating ReflectionAllocator.
Deprecating ReflectionAllocator
is another idea that should be discussed at the Forum/Hub ASAP.
Sorry @stephengold, I thought I have already deleted that comment. I guess it's too late to delete it now but I will adjust it to sense a bit nice;)
You are right deprecating is another topic that should be discussed on the forum.
Regards
Here's the diagnostic message I got when I ran TestReleaseDirectMemory
on 64-bit Linux using OpenJDK 16.0.1:
Dec 08, 2021 12:55:58 PM com.jme3.system.JmeDesktopSystem initialize
INFO: Running on jMonkeyEngine 3.5.0-SNAPSHOT
* Branch: master
* Git Hash: 645b1bb
* Build Date: 2021-12-08
Inconsistency detected by ld.so: dl-lookup.c: 111: check_match: Assertion `version->filename == NULL || ! _dl_name_match_p (version->filename, map)' failed!
Using OpenJDK 14.0.2, I get additional diagnostic messages:
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.jme3.util.ReflectionAllocator (file:/home/sgold/Git/jmonkeyengine/jme3-core/build/libs/jme3-core-3.5.0-SNAPSHOT.jar) to method sun.nio.ch.DirectBuffer.cleaner()
WARNING: Please consider reporting this to the maintainers of com.jme3.util.ReflectionAllocator
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
Dec 08, 2021 1:08:29 PM com.jme3.system.JmeDesktopSystem initialize
INFO: Running on jMonkeyEngine 3.5.0-SNAPSHOT
* Branch: master
* Git Hash: 645b1bb
* Build Date: 2021-12-08
Inconsistency detected by ld.so: dl-lookup.c: 111: check_match: Assertion `version->filename == NULL || ! _dl_name_match_p (version->filename, map)' failed!
After adding the "--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED" JVM argument, the test still crashes similarly with OpenJDK 14.0.2 and even with OpenJDK 11.0.11 . So I don't think this crash has much to do with JDK 16.
@Ali-RS: I need some help reproducing the "Buffer cannot be destroyed" crash. So far, I haven't even managed to create a buffer with anything other than OpenJDK 8.
Inconsistency detected by ld.so: dl-lookup.c: 111: check_match: Assertion `version->filename == NULL || ! _dl_name_match_p (version->filename, map)' failed!
This looks very familiar. This error is typical of binary incompatibility between native libraries and newer builds of the JRE. Last time this came up, I think the only solution was to use lwjgl3, which was built against the newer runtimes.
There may be more details on the hub.
@stephengold can you try with AdoptOpenJDK?
AFAIK LWJGL2 is not compatible with OpenJDK 12+ (also with some versions of OpenJDK 11, IIRC 11.0.6+)
Otherwise, I think you should use LWJGL3 as mentioned by Sailsman63.
I'm sorry I did not clarify this in advance.
Chiming in on @stephengold's request.
I'm running on an Oracle build of JDK 16 with LWJGL 3 with no issues.
@stephengold were you able to reproduce the "Buffer cannot be destroyed" crash?
I never got that far.
A note for anyone who wants to try this with LWJGL3:
LWJGL3 will statically register its own buffer allocator at LwjglContext:
which will be statically initialized at this line into the final field allocator
:
So you must make sure to register ReflectionAllocator
before the static block run, for example, you can specify it as JVM argument in Gradle build:
'-Dcom.jme3.BufferAllocatorImplementation=com.jme3.util.ReflectionAllocator'
BufferAllocator allocator
to get initialized with Reflectionallocator
and bypass the LWJGL3 allocator. :open_mouth:The ReflectionAllocator was an hack to begin with. Does anyone know if it is used outside of the lwjgl2 renderer? If not i assume it will be deprecated with lwjgl2 at some point.
Sometimes the order of these static executions is different, e.g the one in the BufferUtils runs before the LwjglContext, which will cause BufferAllocator allocator to get initialized with Reflectionallocator and bypass the LWJGL3 allocator. open_mouth So best to specify it via JVM args.
It seems we should find a different way to set the allocator, maybe the renderers should have the buffer allocator under the same class name and namespace so that when the modules are switched it gets replaced without setting it "manually".
Does anyone know if it is used outside of the lwjgl2 renderer?
Yes, AndroidBufferAllocator
uses it. It is error-prone on android as well. See https://hub.jmonkeyengine.org/t/androidbufferallocator-and-nonsdkapiusedviolation/45124/2?u=ali_rs
For Android, a workaround may be to write a JNI-based allocator that uses c++ to crete and destroy DirectByteBuffer. Something like:
JNIEXPORT jobject JNICALL allocate(JNIEnv* env, jclass clazz, jint numBytes) {
char* ptr = (char*)malloc(numBytes);
return env->NewDirectByteBuffer(ptr, numBytes);
}
JNIEXPORT void JNICALL destroyDirectBuffer(JNIEnv* env, jobject thiz, jobject bufferRef)
{
void* buffer = env->GetDirectBufferAddress(bufferRef);
free(buffer);
}
another workaround is to use PrimitiveAllocator
which does nothing and lets GC handle the buffer cleanup.
We already have the jme3-android-native module, the JNI allocator can be added there. Using PrimitiveAllocator might be problematic especially with low memory devices.
It seems we should find a different way to set the allocator, maybe the renderers should have the buffer allocator under the same class name and namespace so that when the modules are switched it gets replaced without setting it "manually".
To elaborate on this:
The first part of my proposal is to have a NativeAllocator that is implemented by every rendered module internally (eg. lwjgl3 will use its allocator, lwjgl2 will use ReflectionAllocator, android will use the jni allocator) with the same class on the same path: com.jme3.util.NativeAllocator . In this way when a different renderer module is used its native allocator is loaded.
The next step of the solution is to have BufferAllocatorFactory load the allocator that is specified with the property PROPERTY_BUFFER_ALLOCATOR_IMPLEMENTATION like it does now, but with the default always being com.jme3.util.NativeAllocator, and if the specified class doesn't exist make it fallback to PrimitiveAllocator.
That should solve or provide a path to solve every possible issue. What do you think?
Is NativeAllocator
an interface? or a class that implements the BufferAllocator
interface and should exist in all render modules (lwjgl3, lwjgl, android)?
A class that implements BufferAllocator
Ok, that sounds good to me.
The idea of a NativeAllocator seems good to me too, i will try to implement it for android.
Thank you @Scrappers-glitch
Thank you @Scrappers-glitch
Btw, android internals use the same maneuver under android.hardware.HardwareBuffer
, so we are on the right path :-) :
The native binding for allocating mem on gpu : https://developer.android.com/reference/android/hardware/HardwareBuffer
The native lib : https://developer.android.com/ndk/reference/group/a-hardware-buffer
Sources at Android cs : https://cs.android.com/android/platform/superproject/+/master:external/mesa3d/include/android_stub/android/hardware_buffer.h;l=46?q=hardware_buffer.h&ss=android
@Ali-RS This may be why you have experienced this bug on JDK-16 and it's absent on other jdks, ReflectionAllocator
makes a direct call to sun.*
packages, "Internal method signature are subject to change across jdks" :
https://www.oracle.com/java/technologies/faq-sun-packages.html
And in particular, there has been a serious push to remove the sun.*
packages altogether.
However, if that were the cause you should be getting java.lang.NoClassDefFoundError
, unless it's being swallowed somewhere.
Or guarded against reflective calls...i don't know why generally, but on android they made java reflection and invocation api a black list.
i don't know why generally
Mostly security. Reflection allows access to APIs that may not be safe for general code to use. Even desktop Java is moving to block reflection to a lot of internal packages. But in that case, you should be getting IllegalReflectiveAccess
errors. Again, unless they are being swallowed by code.
I am done with Android Native buffer allocator, but there is something which is inconsistent, on com.jme3.utl
package in the module jme3-android, you will find similar classes on jme3-core but designed for android but prefixed by Android
which is not the case on other packages like com.jme3.input
and com.jme3.system
, they have their classes inside com.jme3,input.android
and com.jme3.system.android
, so i know this is another issue but what should i do about the Android native buffer ? Should i just name it AndroidNativeBufferAllocator
or should i create a package android
under com.jme3.util
and keep the name NativeBufferAllocator
?
Based on riccardobl suggestion:
It seems we should find a different way to set the allocator, maybe the renderers should have the buffer allocator under the same class name and namespace so that when the modules are switched it gets replaced without setting it "manually".
To elaborate on this:
The first part of my proposal is to have a NativeAllocator that is implemented by every rendered module internally (eg. lwjgl3 will use its allocator, lwjgl2 will use ReflectionAllocator, android will use the jni allocator) with the same class on the same path: com.jme3.util.NativeAllocator . In this way when a different renderer module is used its native allocator is loaded.
I suppose we should put it under the com.jme3.util
package and the name it NativeAllocator
or maybe better to name it NativeBufferAllocator
.
Would it create a conflict when we build the NativeBufferAllocator
on desktop because both are at the same package com.jme3.util
?
EDIT :
We should never use NativeAllocator
as a name, it is vague, might imply different things to allocate rather than a buffer..
I just want to know, to avoid conflict with desktop native buffer allocator, should we name the android allocator as AndroidNativeBufferAllocator
or create a new package android
under com.jme3.util
and keep the name NativeBufferAllocator
(but this is an inconsistency with the other classes in the same package, as they all are prefixed with Android
to differentiate them from desktop) ?
However, other android packages like com.jme3.input
and system has a sub folder android and the classes inside aren't prefixed by Android
.
Would it create a conflict when we build the NativeBufferAllocator on desktop because both are at the same package com.jme3.util ?
No, because at the runtime only one of these modules (either lwjgl or android) will be in classpath. On desktop only jme3-lwjgl3 module will be loaded and on android only jme3-android module will be loaded.
By the way, I am not sure if riccardobl approach is the best way or if we can use a better alternative. But this is another topic and we need to discuss this on the forum.
I am ok with either of these namings:
com.jme3.util.AndroidNativeBufferAllocator
or
com.jme3.util.NativeBufferAllocator
I am not sure if riccardobl approach is the best way or if we can use a better alternative. But this is another topic and we need to discuss this on the forum.
I guess its the only way, I read some links of lwjgl describing different approaches, by far handling direct buffers natively is the best way, and btw PrimitiveAllocator
might be also problematic on desktop arm devices as Raspberry PIs (kinda the same memory problems as android), so I think its better also to implement the approach on desktop instead of ReflectionAllocator.
Note, I am talking about this issue:
Sometimes the order of these static executions is different, e.g the one in the BufferUtils runs before the LwjglContext, which will cause BufferAllocator allocator to get initialized with Reflectionallocator and bypass the LWJGL3 allocator. open_mouth So best to specify it via JVM args.
riccardobl sugested to use a different approach to solve the issue of staticly setting allocator in code :
It seems we should find a different way to set the allocator, maybe the renderers should have the buffer allocator under the same class name and namespace so that when the modules are switched it gets replaced without setting it "manually".
maybe an enhancement to this would be to config the allocator name via properties file (for each backend) instead of using a fixed class name for all allocators.
So that BufferAllocatorFactory will read the property PROPERTY_BUFFER_ALLOCATOR_IMPLEMENTATION from the properties file.
Oh, it's better to infer this conversation to the forum, so i haven't looked up on LwjglAllocator
yet, but this solution can be an alternative for natives on desktop ?
but this solution can be an alternative for natives on desktop ?
Sorry not sure if I understand what you mean. LwjglBufferAllocator
in jme3-lwjgl3 is already using the native method (via JNI through lwjgl3) and we are not going to replace it.
But if you are talking about lwjgl2, it does not use a native allocator, it uses RefectionAllocator, so yes we can write a native allocator for lwjgl2 to replace the RefectionAllocator, just like what you did for android.
Thanks for elaborating this, I will take a deep look into both jme3-lwjgl
and jme3-lwjgl3
modules, may be I can help in taking decisions, once I understand what's going on there, I will open a forum discussion to help deciding what is best.
Further discussion on forums : https://hub.jmonkeyengine.org/t/directbufferallocator/45591
There is a new report today about this issue : https://hub.jmonkeyengine.org/t/illegal-reflective-access-warning/45603?u=pavl_g
After diving more and adding some println to the suppressed exceptions I gathered some more info about error:
java.lang.reflect.InaccessibleObjectException: Unable to make public abstract jdk.internal.ref.Cleaner sun.nio.ch.DirectBuffer.cleaner() accessible: module java.base does not "exports sun.nio.ch" to unnamed module @4bca8b50
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354)
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
at java.base/java.lang.reflect.Method.checkCanSetAccessible(Method.java:199)
at java.base/java.lang.reflect.Method.setAccessible(Method.java:193)
at com.jme3.util.ReflectionAllocator.loadMethod(ReflectionAllocator.java:84)
at com.jme3.util.ReflectionAllocator.<clinit>(ReflectionAllocator.java:63)
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:375)
at com.jme3.util.BufferAllocatorFactory.create(BufferAllocatorFactory.java:30)
at com.jme3.util.BufferUtils.<clinit>(BufferUtils.java:65)
at com.jme3.renderer.lwjgl.LwjglGL.<init>(LwjglGL.java:15)
at com.jme3.system.lwjgl.LwjglContext.initContext(LwjglContext.java:283)
at com.jme3.system.lwjgl.LwjglContext.initContextFirstTime(LwjglContext.java:265)
at com.jme3.system.lwjgl.LwjglContext.internalCreate(LwjglContext.java:454)
at com.jme3.system.lwjgl.LwjglAbstractDisplay.initInThread(LwjglAbstractDisplay.java:124)
at com.jme3.system.lwjgl.LwjglAbstractDisplay.run(LwjglAbstractDisplay.java:221)
at java.base/java.lang.Thread.run(Thread.java:833)
java.lang.ClassNotFoundException: sun.misc.Cleaner
at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:520)
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:375)
at com.jme3.util.ReflectionAllocator.loadMethod(ReflectionAllocator.java:83)
at com.jme3.util.ReflectionAllocator.<clinit>(ReflectionAllocator.java:64)
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:375)
at com.jme3.util.BufferAllocatorFactory.create(BufferAllocatorFactory.java:30)
at com.jme3.util.BufferUtils.<clinit>(BufferUtils.java:65)
at com.jme3.renderer.lwjgl.LwjglGL.<init>(LwjglGL.java:15)
at com.jme3.system.lwjgl.LwjglContext.initContext(LwjglContext.java:283)
at com.jme3.system.lwjgl.LwjglContext.initContextFirstTime(LwjglContext.java:265)
at com.jme3.system.lwjgl.LwjglContext.internalCreate(LwjglContext.java:454)
at com.jme3.system.lwjgl.LwjglAbstractDisplay.initInThread(LwjglAbstractDisplay.java:124)
at com.jme3.system.lwjgl.LwjglAbstractDisplay.run(LwjglAbstractDisplay.java:221)
at java.base/java.lang.Thread.run(Thread.java:833)
java.lang.NoSuchMethodException: sun.nio.ch.DirectBuffer.viewedBuffer()
at java.base/java.lang.Class.getMethod(Class.java:2227)
at com.jme3.util.ReflectionAllocator.loadMethod(ReflectionAllocator.java:83)
at com.jme3.util.ReflectionAllocator.<clinit>(ReflectionAllocator.java:65)
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:375)
at com.jme3.util.BufferAllocatorFactory.create(BufferAllocatorFactory.java:30)
at com.jme3.util.BufferUtils.<clinit>(BufferUtils.java:65)
at com.jme3.renderer.lwjgl.LwjglGL.<init>(LwjglGL.java:15)
at com.jme3.system.lwjgl.LwjglContext.initContext(LwjglContext.java:283)
at com.jme3.system.lwjgl.LwjglContext.initContextFirstTime(LwjglContext.java:265)
at com.jme3.system.lwjgl.LwjglContext.internalCreate(LwjglContext.java:454)
at com.jme3.system.lwjgl.LwjglAbstractDisplay.initInThread(LwjglAbstractDisplay.java:124)
at com.jme3.system.lwjgl.LwjglAbstractDisplay.run(LwjglAbstractDisplay.java:221)
at java.base/java.lang.Thread.run(Thread.java:833)
java.lang.reflect.InaccessibleObjectException: Unable to make public abstract java.lang.Object sun.nio.ch.DirectBuffer.attachment() accessible: module java.base does not "exports sun.nio.ch" to unnamed module @4bca8b50
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354)
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
at java.base/java.lang.reflect.Method.checkCanSetAccessible(Method.java:199)
at java.base/java.lang.reflect.Method.setAccessible(Method.java:193)
at com.jme3.util.ReflectionAllocator.loadMethod(ReflectionAllocator.java:84)
at com.jme3.util.ReflectionAllocator.<clinit>(ReflectionAllocator.java:68)
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:375)
at com.jme3.util.BufferAllocatorFactory.create(BufferAllocatorFactory.java:30)
at com.jme3.util.BufferUtils.<clinit>(BufferUtils.java:65)
at com.jme3.renderer.lwjgl.LwjglGL.<init>(LwjglGL.java:15)
at com.jme3.system.lwjgl.LwjglContext.initContext(LwjglContext.java:283)
at com.jme3.system.lwjgl.LwjglContext.initContextFirstTime(LwjglContext.java:265)
at com.jme3.system.lwjgl.LwjglContext.internalCreate(LwjglContext.java:454)
at com.jme3.system.lwjgl.LwjglAbstractDisplay.initInThread(LwjglAbstractDisplay.java:124)
at com.jme3.system.lwjgl.LwjglAbstractDisplay.run(LwjglAbstractDisplay.java:221)
I found out on java 16+ in addition to
"--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED"
I must also add
"--add-exports=java.base/sun.nio.ch=ALL-UNNAMED"
then error SEVERE: Buffer cannot be destroyed: java.nio.DirectFloatBufferU
will go away.
Let's see when it will break again!
Calling the sun.*
is not a good practice though, as stated by sun-package-faq.
I think the right way is to create another issue to refactor ReflectionAllocator to use lwjgl native allocator for instance.
EDIT: I may have outdated information, if this is not fixed yet, we should deprecate the ReflectionAllocator and replace it with a native one (like we did on android), if this is the case, let's continue our discussion on the forum before opening another issue.
we should deprecate the ReflectionAllocator and replace it with a native one (like we did on android), if this is the case, let's continue our discussion on the forum before opening another issue.
Yeah, I have already made a forum post regarding the deprecation of ReflectionAllocator here:
https://hub.jmonkeyengine.org/t/deprecating-reflectionallocator/46350
replace it with a native one (like we did on android)
That would be cool, please let me know in case you are interested to work on this.
Yeah, I will be happy to work on this, but I think if Lwjgl already (which I think it really does) provides a java implementation for a native allocator, we might use it.
I agree with Pavl, that's just workarounds and ultimately this may stop working, the JRE already moved from a warning to an error. So we'd need to find different allocators. Without knowing what it's exact use is, there are probably a lot of different allocators to choose from, so we should do the research work here
but I think if Lwjgl already (which I think it really does) provides a java implementation for a native allocator, we might use it.
I had a quick glance at the lwjgl2 source code from the below link, there is already a BufferUtils and a MemoryUtils there but I could not find a destroy buffer method.
So we'd need to find different allocators. Without knowing what it's exact use is, there are probably a lot of different allocators to choose from, so we should do the research work here
I think for android, there was a discussion around that, and "calloc" is suggested by the core team.
https://github.com/jMonkeyEngine/jmonkeyengine/pull/1821#discussion_r878656286
We should track lwjgl2 native header files to decide whether there is already a destroy criteria there (it must be there), however I cannot find any header file inside lwjgl2-jme ? I hope they are not inside the shared object files (which is a bad practice)....I will try to track them down...
So we'd need to find different allocators. Without knowing what it's exact use is, there are probably a lot of different allocators to choose from, so we should do the research work here
I think for android, there was a discussion around that, and "calloc" is suggested by the core team.
Yes, because calloc = malloc + evaluating the buffer to NULL or zero (memset(0)).
@Scrappers-glitch I think we better create it in a stand alone project separate from the lwjgl2 project . (e.g. in a jme3-calloc
project hosted on JME Github organization) so it can be used outside of lwjgl2 too. You can copy whatever native code you need from lwjgl2. I guess you might have a hard time tackling with lwjgl2 native builds for different OS. Our lwjgl2 build workflow uses precompiled natives.
Not sure though, I would like to know @stephengold opinion about this as well.
@Scrappers-glitch I think we better create it separate from the lwjgl2 project. (e.g. in a
jme3-calloc
project hosted on JME Github organization) You can copy whatever code you need from lwjgl2. I guess you might have a hard time tackling with lwjgl2 native builds for different OS. Our lwjgl2 build workflow uses precompiled natives.I would like to know @stephengold opinion about this as well.
Looks clean, i don't mind to create a new API, however i am not so far good at building natives for cross-platform (i only do linux arches and android), so this might take a lot of time, i am currently learning cmake if it would bring something new to the engine, then let me know...
EDIT: For cmake, you will just have to run the build script and it will generate a native system build file (makefile or visual studio solution) that will use the native compiler for the build process, I think there will be no special configuration, we can create a dummy project to test it with github actions before doing anything.
SEVERE: Buffer cannot be destroyed: java.nio.DirectFloatBufferU
Even by adding this JVM arg (which used to solve that on jdk 11+)
"--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED"
you can test it with
https://github.com/jMonkeyEngine/jmonkeyengine/blob/d76d4dc8c67bc99c5fd197d41a102639194a4e3b/jme3-examples/src/main/java/jme3test/app/TestReleaseDirectMemory.java#L43
Forum post: https://hub.jmonkeyengine.org/t/jme-on-jdk-16/44411/6?u=ali_rs