Buffer allocator implementation set by LwjglContext gets ignored sometimes

Ali_RS · May 21, 2022, 6:10am

Hi

LwjglContext (in jme3-lwjgl3 module) specifies its own implementation of buffer allocator to be used by BufferAllocatorFactory by setting PROPERTY_BUFFER_ALLOCATOR_IMPLEMENTATION property in this static code block:

github.com

jMonkeyEngine/jmonkeyengine/blob/8af0c92991eab9c2027dfd94be09f2170beea630/jme3-lwjgl3/src/main/java/com/jme3/system/lwjgl/LwjglContext.java#L91-L102

      
        
            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());
                    }
                }
            }

the issue is that sometimes this property gets ignored by BufferAllocatorFactory and a ReflectionAllocator is created instead. That is because BufferAllocatorFactory.create() method sometimes is called before the LwjglContext sets the new property. Inside the BufferUtils class:

github.com

jMonkeyEngine/jmonkeyengine/blob/8af0c92991eab9c2027dfd94be09f2170beea630/jme3-core/src/main/java/com/jme3/util/BufferUtils.java#L65

      
        
            * jME data classes such as Vectors and ColorRGBA.

            *

            * @author Joshua Slack

            * @version $Id: BufferUtils.java,v 1.16 2007/10/29 16:56:18 nca Exp $

            */

            public final class BufferUtils {

            

               /**

                * Should be final for thread safety.

                */

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

            

               private static boolean trackDirectMemory = false;

               final private static ReferenceQueue<Buffer> removeCollected = new ReferenceQueue<Buffer>();

               final private static ConcurrentHashMap<BufferInfo, BufferInfo> trackedBuffers = new ConcurrentHashMap<BufferInfo, BufferInfo>();

               static ClearReferences cleanupthread;

            

               /**

                * A private constructor to inhibit instantiation of this class.

                */

               private BufferUtils() {

Sometimes the order of these static code execution 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.

Edit:
2 possible fixes are suggested for this issue:

github.com/jMonkeyEngine/jmonkeyengine

ReflectionAllocator is broken on JDK 16

opened 07:09PM - 23 Nov 21 UTC

Ali-RS

bug To be investigated

`SEVERE: Buffer cannot be destroyed: java.nio.DirectFloatBufferU` Even by add…ing 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

and:

github.com/jMonkeyEngine/jmonkeyengine

ReflectionAllocator is broken on JDK 16

opened 07:09PM - 23 Nov 21 UTC

Ali-RS

bug To be investigated

`SEVERE: Buffer cannot be destroyed: java.nio.DirectFloatBufferU` Even by add…ing 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

Ali_RS · May 21, 2022, 6:14am

I have also submitted an issue to GitHub:

github.com/jMonkeyEngine/jmonkeyengine

PROPERTY_BUFFER_ALLOCATOR_IMPLEMENTATION set by LwjglContext gets ignored sometimes

opened 06:08AM - 21 May 22 UTC

Ali-RS

bug

Hi LwjglContext (in jme3-lwjgl3 module) specifies its own implementation of b…uffer allocator to be used by [BufferAllocatorFactory](https://github.com/jMonkeyEngine/jmonkeyengine/blob/f0918a4caf4eb592add2221243afb4dfcf13ca28/jme3-core/src/main/java/com/jme3/util/BufferAllocatorFactory.java) by setting `PROPERTY_BUFFER_ALLOCATOR_IMPLEMENTATION` property in this static code block: https://github.com/jMonkeyEngine/jmonkeyengine/blob/8af0c92991eab9c2027dfd94be09f2170beea630/jme3-lwjgl3/src/main/java/com/jme3/system/lwjgl/LwjglContext.java#L91-L102 the issue is that sometimes this property gets ignored by BufferAllocatorFactory and a ReflectionAllocator is created instead. That is because `BufferAllocatorFactory.create()` method sometimes is called before the LwjglContext sets the new property. Inside the BufferUtils class: https://github.com/jMonkeyEngine/jmonkeyengine/blob/8af0c92991eab9c2027dfd94be09f2170beea630/jme3-core/src/main/java/com/jme3/util/BufferUtils.java#L65 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.

Ali_RS · May 21, 2022, 6:19am

We might have the same issue in android OGLESContext as well:

github.com

jMonkeyEngine/jmonkeyengine/blob/8af0c92991eab9c2027dfd94be09f2170beea630/jme3-android/src/main/java/com/jme3/system/android/OGLESContext.java#L81-L87

      
        
            static {
                final String implementation = BufferAllocatorFactory.PROPERTY_BUFFER_ALLOCATOR_IMPLEMENTATION;
            
            
    if (System.getProperty(implementation) == null) {
                    System.setProperty(implementation, AndroidBufferAllocator.class.getName());
                }
            }

RiccardoBlb · May 21, 2022, 7:10am

The fix i proposed is similar to what the engine already uses for the backend support, for example, so i think we should go with that.

I will look into it, but first we need to get this merged

Ali_RS · May 21, 2022, 7:23am

So should the current buffer allocators get renamed to NativeBufferAllocator?

For example, jme3-lwjgl3 has 2 native buffer allocators, LWJGLBufferAllocator and ConcurrentLWJGLBufferAllocator which extends LWJGLBufferAllocator. ConcurrentLWJGLBufferAllocator is used by default.
How do you want to rename these classes?

RiccardoBlb · May 21, 2022, 7:30am

Probably we should have NativeBufferAllocator as a third class that uses either LWJGLBufferAllocator or ConcurrentLWJGLBufferAllocator(default) depending on a system property that is passed from the command line

Ali_RS · May 21, 2022, 10:00am

To me, using a config file (e.g. bufferallocator.cfg) seems a bit cleaner.

For example in the jme3-lwjgl3 module the file will contain:

com.jme3.BufferAllocatorImplementation=com.jme3.util.LWJGLBufferAllocator$ConcurrentLWJGLBufferAllocator

Then BufferAllocatorFactory will first check for a user-specified property using System.getProperty(PROPERTY_BUFFER_ALLOCATOR_IMPLEMENTATION) and if nothing is specified then it will check the bufferallocator.cfg file.

No extra wrapper classes.

RiccardoBlb · May 21, 2022, 5:42pm

I am not sure, i think the other approach is cleaner, also easier to figure out if you are looking at the code for the first time and it is already used throughout the engine.

Regarding the wrapper class, if we think it through is it there even a reason to keep a non concurrent allocator? We should probably keep only the concurrent one.

Ali_RS · May 21, 2022, 5:57pm

Well in the future, we might add other allocators. For example, the JEP 419: Foreign Function & Memory API will introduce a new API that we can use to release direct byte buffer memory purely in Java so then using the name NativeBufferAllocator for this would not make sense also if we decided to make it the default for one of the backends. But using a config file we can just replace the string to switch the allocator.

RiccardoBlb · May 22, 2022, 6:53am

Mhh, ok. I agree with your proposal.