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

ReflectionAllocator is broken on JDK 16 #1674

Open
Ali-RS opened this issue Nov 23, 2021 · 76 comments
Open

ReflectionAllocator is broken on JDK 16 #1674

Ali-RS opened this issue Nov 23, 2021 · 76 comments
Labels
bug Something that is supposed to work, but doesn't. More severe than a "defect".

Comments

@Ali-RS
Copy link
Member

Ali-RS commented Nov 23, 2021

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

public class TestReleaseDirectMemory extends SimpleApplication {

Forum post:
https://hub.jmonkeyengine.org/t/jme-on-jdk-16/44411/6?u=ali_rs

@Ali-RS Ali-RS added bug Something that is supposed to work, but doesn't. More severe than a "defect". To be investigated labels Nov 23, 2021
@Ali-RS
Copy link
Member Author

Ali-RS commented Nov 23, 2021

Another idea might be to consider deprecating ReflectionAllocator.

@stephengold
Copy link
Member

Deprecating ReflectionAllocator is another idea that should be discussed at the Forum/Hub ASAP.

@Ali-RS
Copy link
Member Author

Ali-RS commented Nov 24, 2021

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

@stephengold
Copy link
Member

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!

@stephengold
Copy link
Member

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.

@stephengold
Copy link
Member

@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.

@Sailsman63
Copy link
Contributor

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.

@Ali-RS
Copy link
Member Author

Ali-RS commented Dec 9, 2021

@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.

@danielperano
Copy link
Member

Chiming in on @stephengold's request.

I'm running on an Oracle build of JDK 16 with LWJGL 3 with no issues.

@Ali-RS
Copy link
Member Author

Ali-RS commented Dec 15, 2021

@stephengold were you able to reproduce the "Buffer cannot be destroyed" crash?

@stephengold
Copy link
Member

I never got that far.

@Ali-RS
Copy link
Member Author

Ali-RS commented Dec 15, 2021

A note for anyone who wants to try this with LWJGL3:

LWJGL3 will statically register its own buffer allocator at LwjglContext:

static {
final String implementation = BufferAllocatorFactory.PROPERTY_BUFFER_ALLOCATOR_IMPLEMENTATION;
if (System.getProperty(implementation) == null) {
if (Boolean.parseBoolean(System.getProperty(PROPERTY_CONCURRENT_BUFFER_ALLOCATOR, "true"))) {
System.setProperty(implementation, ConcurrentLWJGLBufferAllocator.class.getName());
} else {
System.setProperty(implementation, LWJGLBufferAllocator.class.getName());
}
}
}

which will be statically initialized at this line into the final field allocator:

private static final BufferAllocator allocator = BufferAllocatorFactory.create();

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'
  • Off-topic:
    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. 😮
    So best to specify it via JVM args.

@riccardobl
Copy link
Member

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".

@Ali-RS
Copy link
Member Author

Ali-RS commented Apr 3, 2022

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.

@riccardobl
Copy link
Member

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.

@riccardobl
Copy link
Member

riccardobl commented Apr 3, 2022

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?

@Ali-RS
Copy link
Member Author

Ali-RS commented Apr 3, 2022

Is NativeAllocator an interface? or a class that implements the BufferAllocator interface and should exist in all render modules (lwjgl3, lwjgl, android)?

@riccardobl
Copy link
Member

A class that implements BufferAllocator

@Ali-RS
Copy link
Member Author

Ali-RS commented Apr 3, 2022

Ok, that sounds good to me.

@Scrappers-glitch
Copy link
Contributor

The idea of a NativeAllocator seems good to me too, i will try to implement it for android.

@Ali-RS
Copy link
Member Author

Ali-RS commented May 3, 2022

Thank you @Scrappers-glitch

@Scrappers-glitch
Copy link
Contributor

@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

@Sailsman63
Copy link
Contributor

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.

@Scrappers-glitch
Copy link
Contributor

Or guarded against reflective calls...i don't know why generally, but on android they made java reflection and invocation api a black list.

@Sailsman63
Copy link
Contributor

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.

@Scrappers-glitch
Copy link
Contributor

Scrappers-glitch commented May 17, 2022

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 ?

@Ali-RS
Copy link
Member Author

Ali-RS commented May 17, 2022

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.

@Scrappers-glitch
Copy link
Contributor

Once all architectures are built into jme-alloc.jar, other tasks like uploading to sonatype will be simple...

@Ali-RS
Copy link
Member Author

Ali-RS commented Jan 11, 2023

No problem, take your time and ask it on the forum if you need help.

@Ali-RS Ali-RS modified the milestones: Future Release, v3.6.0 Jan 11, 2023
@Scrappers-glitch
Copy link
Contributor

No problem, take your time and ask it on the forum if you need help.

Okay, unless there is an objection to my proposal (using CMake + gradle), i am going to start soon and i will open a forum thread to keep the community updated with the latest status about jme-alloc.

@MeFisto94
Copy link
Member

MeFisto94 commented Jan 12, 2023

Before we build our own native dependency to only call malloc we should really just use one of the widely available allocators already.

One that is available to jwgl3 is jemalloc: https://javadoc.lwjgl.org/org/lwjgl/system/jemalloc/JEmalloc.html
There's also https://netty.io/wiki/using-as-a-generic-library.html
Also considering that the only reason we have the ReflectionAllocator is that we don't want to wait for the direct buffer to be freed by the Garbage Collector (which is because an access to an already freed byte buffer may be catastrophic).
This is still a very active topic in the java world (https://stackoverflow.com/questions/3496508/deallocating-direct-buffer-native-memory-in-java-for-jogl)

Especially the hint to project panama is interesting, if you have a completely new API also supporting try-with-resources.
Quoting https://openjdk.org/jeps/370:

Moreover, working with direct buffers can be cumbersome since deallocation of the memory associated with direct buffers is left to the garbage collector; that is, only after a direct buffer is deemed unreachable by the garbage collector can the associated memory be released.
[...]
While using JNI to access memory is also a possibility, the inherent costs associated with this solution make it seldom applicable in practice. The overall development pipeline is complex, since JNI requires the developer to write and maintain snippets of C code. JNI is also inherently slow, since a Java-to-native transition is required for each access.

And most importantly:

Alternatives
Keep using existing APIs such as ByteBuffer and Unsafe or, worse, JNI.

-> So the Java Devs consider JNI (us wrapping malloc) worse than unsafe/byte buffer. Keep in mind a java allocator could allocate bigger byte buffers manually and then only giving out chunks. But then, we only use it for Megabyte Sized Textures (well and small vert/index buffers.... Could probably also pool them)

So in general, I can't recommend anything right now, but if our buffers are mainly to communicate with openGL, memory mapping with a new API may be interesting. And a specialized allocator may make a difference, too. With malloc I don't know how thread safety looks like etc and you'd still need to get the native address of the byte buffer and try to wrap it etc.

Furthermore, I wouldn't recommend cmake for something so simple, since you need visual studio and the cmake workload on windows, on mac os probably XCode (my mac can't even use any recent XCode anymore because it's "old") etc.
If it only targets CI, may even cross compile on linux (or have very simple manual gcc calls), or try out the Gradle Support for C++ projects, as seems common on Android anyway.

@Scrappers-glitch
Copy link
Contributor

Scrappers-glitch commented Jan 12, 2023

@MeFisto94 Yes, you are right, we shouldn't create a new API for just malloc(), if using jemalloc will be enough then let me know.

About multi-threading and thread safety in jni:

I think i didn't respect this point while building android native buffer so far, so thank you for pointing this out, i totally forgot.

@Ali-RS
Copy link
Member Author

Ali-RS commented Jan 12, 2023

if using jemalloc will be enough then let me know.

I am okay using lwjgl3 jemalloc lib. Can we use it without depending on lwjgl3-core?

@Scrappers-glitch
Copy link
Contributor

if using jemalloc will be enough then let me know.

I am okay using lwjgl3 jemalloc lib. Can we use it without depending on lwjgl3-core?

I think it cannot, this is a script generated by lwjgl.org/customize.

import org.gradle.internal.os.OperatingSystem

project.ext.lwjglVersion = "3.3.1"

switch (OperatingSystem.current()) {
	case OperatingSystem.LINUX:
		def osArch = System.getProperty("os.arch")
		project.ext.lwjglNatives = osArch.startsWith("arm") || osArch.startsWith("aarch64")
			? "natives-linux-${osArch.contains("64") || osArch.startsWith("armv8") ? "arm64" : "arm32"}"
			: "natives-linux"
		break
	case OperatingSystem.MAC_OS:
		project.ext.lwjglNatives = System.getProperty("os.arch").startsWith("aarch64") ? "natives-macos-arm64" : "natives-macos"
		break
	case OperatingSystem.WINDOWS:
		def osArch = System.getProperty("os.arch")
		project.ext.lwjglNatives = osArch.contains("64")
			? "natives-windows${osArch.startsWith("aarch64") ? "-arm64" : ""}"
			: "natives-windows-x86"
		break
}

repositories {
	mavenCentral()
}

dependencies {
	implementation platform("org.lwjgl:lwjgl-bom:$lwjglVersion")

	implementation "org.lwjgl:lwjgl"
	implementation "org.lwjgl:lwjgl-jemalloc"
	runtimeOnly "org.lwjgl:lwjgl::$lwjglNatives"
	runtimeOnly "org.lwjgl:lwjgl-jemalloc::$lwjglNatives"
}

@Scrappers-glitch
Copy link
Contributor

Scrappers-glitch commented Jan 12, 2023

Now, i am totally sure it cannot be used without lwjgl-core, it utilizes the lwjgl-core system package:
https://github.com/LWJGL/lwjgl3/blob/ee2dc3bbdc8d601428c1108cf8486e2aaf18fd07/modules/lwjgl/jemalloc/src/generated/java/org/lwjgl/system/jemalloc/JEmalloc.java#L12-L20

@Ali-RS
Copy link
Member Author

Ali-RS commented Jan 12, 2023

Just my two cents!

If you think it is possible to rip only the required java code + natives from lwjgl3 then go that way but if you think creating our own thin API around malloc/calloc would be easier, then do that way, or if you think it would be easy to use GetDirectBufferAddress from lwjgl2 code to get java ByteBufer memory address and destroy it with stdlib then use that.

You can use whatever build tool you are familiar with it does not matter. What matters is that we want a jme3-alloc.jar with natives inside and a java class that implements BufferAllocator:

/**
* Interface to create/destroy direct buffers.
*/
public interface BufferAllocator {
/**
* De-allocate a direct buffer.
*
* @param toBeDestroyed the buffer to de-allocate (not null)
*/
void destroyDirectBuffer(Buffer toBeDestroyed);
/**
* Allocate a direct ByteBuffer of the specified size.
*
* @param size in bytes (≥0)
* @return a new direct buffer
*/
ByteBuffer allocate(int size);
}

The jar should be hosted on maven central (either under org.jmonkeyengine or @Scrappers-glitch own organization) and then we will add a dependency to it in jme3-lwjgl.

Simillar to what @stephengold did with https://github.com/stephengold/j-ogg-all/ and https://github.com/stephengold/stack-alloc

Edit:

You can also take a look at how LibGdx does this in there buffer allocator:
https://github.com/libgdx/libgdx/blob/28223b6169a99ab063b2b253fb0b1ebf264d0eca/gdx/src/com/badlogic/gdx/utils/BufferUtils.java#L559-L567

@Scrappers-glitch
Copy link
Contributor

Scrappers-glitch commented Jan 12, 2023

if you think it would be easy to use GetDirectBufferAddress from lwjgl2 code to get java ByteBufer memory address and destroy it with stdlib then use that.

I realized that the code we implemented on android is really doing this indeed :-), but this option renders the same solution as our own API, we will have to recompile sources.

It's not possible to separate jemalloc from lwjgl3-core and lwjgl3-natives.

@MeFisto94
Copy link
Member

In theory, an allocator is the backbone of an application, so don't over-rush this please.
As to thread safety, e.g, can you safely call free() on a memory block that has been malloc'ed() on another thread?
Can you free() a memory block that has not been malloced (e.g. JVM's direct buffers could use the platform native API such as HeapAlloc on Windows).

Tuning the allocator is also yet another topic.
Also, if jemalloc doesn't work, there are hundreds of other allocators. I really like Netty's ByteBuf, e.g., but it's not API compatible with Java Standard ByteBuffers.

@Scrappers-glitch
Copy link
Contributor

As to thread safety, e.g, can you safely call free() on a memory block that has been malloc'ed() on another thread?
Can you free() a memory block that has not been malloced (e.g. JVM's direct buffers could use the platform native API such as HeapAlloc on Windows).

AFAIK, malloc() allocates new memory on the native heap which is visible to the jvm, use JavaVM instead of local JNIEnv to call the GetAddress method locally inside the native method.

@pspeed42
Copy link
Contributor

Visible but not managed by. I don't know if this is really a concern or not.

One of the big problems with Java6 was having to specify a huge direct heap because GC would free direct memory but did not consider it when deciding to run GC. There is a concern that if we move to memory that Java does not manage that maybe we return to those awful days. That would be unfortunate.

I admit I have not looked into what's going on here, though. Not really something to take lightly in the alpha phase of a piece of software maybe.

@stephengold
Copy link
Member

can you safely call free() on a memory block that has been malloc'ed() on another thread?

Any modern C (or C++) compiler should provide a runtime library with thread-safe malloc()/free().

@Scrappers-glitch
Copy link
Contributor

Scrappers-glitch commented Jan 14, 2023

Okay, i think we are left with an additional option which is creating a new minimalistic allocation api (BufferUtils for example) on top of Libbulletjme hosted by @stephengold.

@stephengold Let me know your opinion about this option if this is feasible, and also if this will cause any conflict for people who still use other bullet physics libraries, if we are good with this option, assign me to this and i will work on a PR to the libbulletjme to add the BufferUtils and another PR to the engine to utilize a new NativeDesktopAllocator.

@stephengold
Copy link
Member

stephengold commented Jan 14, 2023

  • Libbulletjme is a huge amount of code (253K lines of source, 370KB class JAR + a 3 MB native for each platform). I sincerely doubt JMonkeyEngine wants it as a dependency.
  • Minie and Libbulletjme define many of the same Java classes. Adding Libbulletjme as a dependency of JMonkeyEngine would break Minie in ways that would be difficult to fix.
  • While Libbulletjme implements automatic deallocation of (native) physics objects, it doesn't do so for direct buffers. For buffers, it uses a copy of JMonkeyEngine's PrimitiveAllocator---it never deallocates direct buffers!

I'm willing to create a new library for buffer allocation and deallocation, using a weak-reference tracking mechanism similar to what Libbulletjme uses for physics objects. However, I don't have a clear sense of how long it would take, and I'm unclear how much time remains before the JME 3.6 code freeze.

@Ali-RS
Copy link
Member Author

Ali-RS commented Jan 14, 2023

and I'm unclear how much time remains before the JME 3.6 code freeze.

It can be a separate project independent of JME releases.

@Scrappers-glitch
Copy link
Contributor

Okay, i have a raw Algorithm library arithmos, it's a standalone project with no dependencies, i will just refactor it to use any of gradle-jni-templates to get myself familiar with this until we reach a solution, i guess what will take most of the time (if not all) is building to windows and Mac systems, otherwise coding will not be a very hard task.

@Ali-RS
Copy link
Member Author

Ali-RS commented Jan 14, 2023

Note that the jme3-alloc library should only contain the code and natives required for buffer allocation and deallocation. It should not contains other stuff related to physics, image loading,...!

@stephengold
Copy link
Member

But first, I'm unclear why we even need ReflectionAllocator. What are the circumstances under which one cannot use PrimitiveAllocator?

@Scrappers-glitch
Copy link
Contributor

May be using low-memory desktop devices like raspberries or Risc-V MCU ? It may give us some memory performance.

@stephengold
Copy link
Member

Related discussion at the Discourse Forum/Hub: https://hub.jmonkeyengine.org/t/jme-alloc-project/46356

@Scrappers-glitch
Copy link
Contributor

May be using low-memory desktop devices like raspberries or Risc-V MCU ? It may give us some memory performance.

I was wrong !

This project aims to provide a backward compatible direct memory allocation api for desktop on lwjgl-2.

Currently, I managed to build x86-64 binaries for Linux, Mac and windows.

I cannot build x86 binaries; because github-runners don't provide x86 systems, so there are 2 options, either to use another CI to build the other variants or create workarounds by downloading their toolchains in their respective systems (so for example downloading GCC for x86 Intel or looking around GCC options to provide a backward compatibility for the output binary).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that is supposed to work, but doesn't. More severe than a "defect".
Projects
None yet
Development

No branches or pull requests

8 participants