Deprecating ReflectionAllocator?

Ali_RS · January 11, 2023, 11:43am

To make the long story short, after java 11, the ReflectionAllocator keeps breaking again and again! (At least it does on Linux, I do not know about Windows or Mac)

SEVERE: Buffer cannot be destroyed: java.nio.DirectFloatBufferU

By a glance at this class source code, you can see the code smells!

github.com/jMonkeyEngine/jmonkeyengine

ReflectionAllocator is broken on JDK 16

opened 07:09PM - 23 Nov 21 UTC

closed 10:41AM - 11 Jan 23 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

with java 11+ we are required to add this VM option to fix it

"--add-opens=java.base/jdk.internal.ref=ALL-UNNAMED"

but it broke up again in java 16+, which now requires also adding:

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

I do not know when it will break again.

But should we expect developers to know about this and add them? I believe not!

Lwjgl3 already has its own native allocator, and thanks to the contribution from @Pavl_G, Android now has its own native allocator in the JME 3.6 too. Leaving out iOS, only lwjgl2 now uses ReflectionAllocator.

With the behind the scene works that we have done on preparing lwjgl2 to run on java 11+ on Linux, the ReflectionAllocator is the only barrier left I believe.

I am proposing to deprecate ReflectionAllocator and make the PrimitiveAllocator the default one, which will let the GC clean the buffers on lwjgl2. I believe this is no more a concern in today’s java, especially with the arrival of ZGC and Shenandoah GC.

If someone has any feedback or concerns about this please let me know here before it is too late!

Edit:

Here is the test case if you want to try it with java 11+

github.com

jMonkeyEngine/jmonkeyengine/blob/ceb356462aeedf7769dced52e1d5572b01deca31/jme3-examples/src/main/java/jme3test/app/TestReleaseDirectMemory.java#L43


      
          package jme3test.app;
          
          
import com.jme3.app.SimpleApplication;
          import com.jme3.material.Material;
          import com.jme3.scene.Geometry;
          import com.jme3.scene.shape.Box;
          import com.jme3.util.BufferUtils;
          import java.nio.ByteBuffer;
          import java.nio.FloatBuffer;
          
          
public class TestReleaseDirectMemory extends SimpleApplication {
          
          
    public static void main(String[] args){
                  TestReleaseDirectMemory app = new TestReleaseDirectMemory();
                  app.start();
              }
          
          
    @Override
              public void simpleInitApp() {
                  Box b = new Box(1, 1, 1);
                  Geometry geom = new Geometry("Box", b);

Regards

pspeed · January 11, 2023, 11:55am

When you say the “default one”, you mean that JME specifically declares a different one? Not that applications must choose.

Just making sure that I understand.

Ali_RS · January 11, 2023, 12:00pm

Yes, JME specifies “ReflectionAllocator” by default in BufferAllocatorFactory but applications can still overwrite it by passing a system property.

github.com

jMonkeyEngine/jmonkeyengine/blob/f0918a4caf4eb592add2221243afb4dfcf13ca28/jme3-core/src/main/java/com/jme3/util/BufferAllocatorFactory.java#L28


      
          
          
    /**
               * A private constructor to inhibit instantiation of this class.
               */
              private BufferAllocatorFactory() {
              }
          
          
    @Internal
              protected static BufferAllocator create() {
          
          
        final String className = System.getProperty(PROPERTY_BUFFER_ALLOCATOR_IMPLEMENTATION, ReflectionAllocator.class.getName());
                  try {
                      return (BufferAllocator) Class.forName(className).getDeclaredConstructor().newInstance();
                  } catch (final Throwable e) {
                      LOGGER.log(Level.WARNING, "Unable to access {0}", className);
                      return new PrimitiveAllocator();
                  }
              }
          }

pspeed · January 11, 2023, 12:11pm

I may have to try setting that system property on my own apps to see if it’s harmful. I do a LOT of allocating. lol.

Ali_RS · January 11, 2023, 12:18pm

This is the source code of PrimitiveAllocator that I am proposing to pick by default.

But we must remove that System.err.println(); warning thought, to prevent spamming console!

github.com

jMonkeyEngine/jmonkeyengine/blob/ceb356462aeedf7769dced52e1d5572b01deca31/jme3-core/src/main/java/com/jme3/util/PrimitiveAllocator.java#L41


      
          */

          package com.jme3.util;

          

          import java.nio.Buffer;

          import java.nio.ByteBuffer;

          

          /**

          * This class contains a primitive allocator with no special logic, should work

          * on any jvm

          */

          public final class PrimitiveAllocator implements BufferAllocator {

             /**

              * De-allocate a direct buffer.

              *

              * @param toBeDestroyed ignored

              */

             @Override

             public void destroyDirectBuffer(Buffer toBeDestroyed) {

                 // no exception by intent, as this way naively written java7/8

                 // applications won't crash on 9 assuming they can dispose buffers

                 System.err.println("Warning destroyBuffer not supported");

Ali_RS · January 11, 2023, 12:32pm

Are you manually destroying them using BufferUtils.destroyDirectBuffer()?

pspeed · January 11, 2023, 1:00pm

No. Is that the only case where this comes up? I thought JME was doing its own buffer freeing or something.

I gave up manual deallocation a long time ago. Since Java 1.7 it hasn’t seemed to be an issue at all.

Ali_RS · January 11, 2023, 1:09pm

Well, I believe yes, if they are wrapped in JME NativeObject like VertexBuffer,…

Pavl_G · January 11, 2023, 6:34pm

@Ali_RS, i tracked the lwjgl2 native sources includes and this is the header utilized by jni:

github.com

jMonkeyEngine/lwjgl2/blob/master/src/native/common/common_tools.h

/*
 * Copyright (c) 2002-2008 LWJGL Project
 * All rights reserved.
 *
 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions are
 * met:
 *
 * * Redistributions of source code must retain the above copyright
 *   notice, this list of conditions and the following disclaimer.
 *
 * * Redistributions in binary form must reproduce the above copyright
 *   notice, this list of conditions and the following disclaimer in the
 *   documentation and/or other materials provided with the distribution.
 *
 * * Neither the name of 'LWJGL' nor the names of
 *   its contributors may be used to endorse or promote products derived
 *   from this software without specific prior written permission.
 *
 * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS

This file has been truncated. show original

The problem is it creates a new direct buffer using the jni function (NewDirectBuffer) from the java nio api, and currently there is no a native destructor method, neither in lwjgl-2 nor in java NIO API, however there is a GetDirectBufferAddress which returns a pointer that i think can be freed using the GNU standard library (stdlib), so i think we can implement this…

EDIT:
This is the documentation for the GetDirectBufferAddress:

GetDirectBufferAddress
void* GetDirectBufferAddress(JNIEnv* env, jobject buf);

Fetches and returns the starting address of the memory region referenced by the given direct java.nio.Buffer.

This function allows native code to access the same memory region that is accessible to Java code via the buffer object.

LINKAGE:
Index 230 in the JNIEnv interface function table.

PARAMETERS:
env: the JNIEnv interface pointer

buf: a direct java.nio.Buffer object (must not be NULL)

RETURNS:
Returns the starting address of the memory region referenced by the buffer. Returns NULL if the memory region is undefined, if the given object is not a direct java.nio.Buffer, or if JNI access to direct buffers is not supported by this virtual machine.

Pesegato · January 12, 2023, 9:55am

So why are we STILL clinging at lwjgl2, which is practically dead, even when releasing a new version? The body can be hidden, but the rotting smell will keep haunting us…

Yes I know there are N reasons for doing so, unfortunately…

Ali_RS · January 15, 2023, 8:02am

I drafted a PR

It does not deprecate ReflectionAllocator just makes PrimitiveAllocator the default one.