jMonkeyEngine / jmonkeyengine

A complete 3-D game development suite written in Java.
http://jmonkeyengine.org
BSD 3-Clause "New" or "Revised" License
3.74k stars 1.12k forks source link

LWJGLBufferAllocator does not initialize new buffers to zeros, ReflectionAllocator does #2176

Closed JosiahGoeman closed 5 months ago

JosiahGoeman commented 6 months ago

ReflectionAllocator uses java.nio.ByteByffer.allocateDirect() which always initializes new buffers to all zeros.

LWJGLBufferAllocator uses org.lwjgl.system.MemoryUtil.nmemAlloc() which does not make the same guarantee. (nmemAlloc doesn't have much documentation, just redirects to this memAlloc description)

Strangely, it seems that this discrepancy only starts manifesting after buffers have been destroyed, so it isn't immediately obvious. Since the uninitialized data is overwritten immediately 99% of the time, it's not a big deal, but my understanding is that LWJGLBufferAllocator is intended to be a drop-in replacement for ReflectionAllocator.

Here's an MRE:

public static void main(String[] args)
    {
        //comment this line out to use ReflectionAllocator
        System.setProperty(BufferAllocatorFactory.PROPERTY_BUFFER_ALLOCATOR_IMPLEMENTATION, LWJGLBufferAllocator.class.getName());

        FloatBuffer buff = BufferUtils.createFloatBuffer(1_000_000);

        //strangely, this one is always(?) zero'd
        for(int i = 0; i < buff.capacity(); i++)
            assert buff.get(i) == 0f;

        //put some random garbage into it and destroy it
        for(int i = 0; i < buff.capacity(); i++)
            buff.put(i, FastMath.rand.nextFloat());
        BufferUtils.destroyDirectBuffer(buff);

        //create another buffer, should contain all zeros like the first one
        FloatBuffer buff2 = BufferUtils.createFloatBuffer(1_000_000);

        //using ReflectionAllocator == A-OK
        //using LWJGLBufferAllocator == crash
        for(int i = 0; i < buff2.capacity(); i++)
            assert buff2.get(i) == 0f;
    }

Possible fix could be to add a MemoryUtil.memSet() call after nmemAlloc(). I can test it out and make a PR later if y'all want.

stephengold commented 6 months ago

This seems worth documenting, at least.

riccardobl commented 6 months ago

Nice find. I think this should be absolutely fixed with memSet as you proposed, or also by using memCalloc. There isn't undefined memory in java, so this behavior in jme is unexpected and can cause side effects that are very hard to debug and notice.

pspeed42 commented 6 months ago

memCalloc is what I'd recommend... but only because the old grizzled C programmers with the horror that lurks deep in their eyes all switched to calloc decades ago...