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

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

Closed
JosiahGoeman opened this issue Jan 3, 2024 · 3 comments · Fixed by #2179
Closed

Comments

@JosiahGoeman
Copy link
Contributor

JosiahGoeman commented Jan 3, 2024

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
Copy link
Member

This seems worth documenting, at least.

@riccardobl
Copy link
Member

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
Copy link
Contributor

pspeed42 commented Jan 3, 2024

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

JosiahGoeman added a commit to JosiahGoeman/jmonkeyengine that referenced this issue Jan 4, 2024
… of nmemAlloc()

jMonkeyEngine#2176
LWJGLBufferAllocator.allocate() now always returns a zero-initialized buffer.  This behavior is consistent with how ReflectionAllocator works, and Java in general.
JosiahGoeman added a commit to JosiahGoeman/jmonkeyengine that referenced this issue Jan 4, 2024
… of nmemAlloc()

jMonkeyEngine#2176
LWJGLBufferAllocator.allocate() now always returns zero-initialized buffers.
scenemax3d pushed a commit that referenced this issue Jan 19, 2024
…() (#2179)

#2176
LWJGLBufferAllocator.allocate() now always returns zero-initialized buffers.
stephengold pushed a commit that referenced this issue Oct 25, 2024
* #2176 Make LWJGLBufferAllocator use nmemCalloc() instead of nmemAlloc()

#2176
LWJGLBufferAllocator.allocate() now always returns zero-initialized buffers.

* Added unit tests for JmeExporter/JmeImporter implementations

Tests all write* and read* methods of OutputCapsule and InputCapsule respectively.

* Fixed XMLExporter/XMLImporter BitSets

Previously DOMOutputCapsule was writing indices of set bits and DOMInputCapsule was reading values of each bit.  Changed DOMOutputCapsule to match the expected behavior of DOMInputCapsule.

* Fixed DOMInputCapsule.readString() returning defVal for empty strings

org.w3c.dom.Element.getAttribute() returns an empty string for attributes that are not found.  It looks like DOMInputCapsule.readString() was interpreting an empty string as the attribute not existing, and returning defVal even when the attribute did exist and was an empty string.  Now it checks explicitly whether the attribute exists.

* Deprecated DOMSerializer in favor of javax.xml.transform.Transformer

DOMSerializer contains several edge-case issues that were only partially worked around with the encodeString() and decodeString() helper methods.  Java has a robust built-in solution to serializing Document objects, and using that instead fixes several bugs.

* Fixed NullPointerException when XMLExporter writes a String[] with null

Also refactored all primitive array write and read methods to be more readable and reduce duplicate code.

* Made DOM capsules reuse write/read primitive array methods for buffers

Further reduces duplicate code

* Fixed DOMOutputCapsule.write(Savable[][]) NullPointerException

Refactored write and read methods for Savables and 1 and 2 dimensional Savable arrays.  Fixed NullPointerException when writing a 2d Savable array containing a null element in the outer array.

* Added Savable reference consistency test to InputOutputCapsuleTest

* Fixed DOMInputCapsule throwing NullPointerException when reading list

DOMInputCapsule used to throw a NullPointerException when reading an Arraylist containing a null element.  Also refactored list write and read methods to clean up a bit and accidentally also fixed an unrelated bug where reading ArrayList<ByteBuffer> would return a list containing all null elements.

* Made XMLExporter save and load buffer positions properly.

* Cleanup and formatting for XMLExporter related classes

* Undid XMLExporter saving buffer positions.

Not saving positions is intentional #2312 (comment)

* Fixed infinite recursion with XMLExporter

Writing a Savable containing a reference loop caused infinite recursion due to bookkeeping being performed after the recursive call instead of before.  Also added a unit test for this to InputOutputCapsuleTest.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants