Fix memory leaks caused by reused arrays of Spatials

[parysto]

If (multiple) Geometries with Textures are being attached to and detached from the Sceengraph at runtime (in update(...)), the Texture memory counter increases continuously and does not reach 0 again. This happens especially after Spatial#getWorldTranslation() has been called.
Even after detaching the Geometries' parent node, cleaning the asset-cache and calling System.gc(), it does not go back to 0.

After some debugging, I found out that this is caused by how ref-variables are used to improve performance:

• Spatial#checkDoTransformUpdate() uses a Spatial[] from TempVars, but does not clean up the references.
• ListSort#sort(...) performs a merge-sort if there are more than 128 entries. The merge sort stores a reference to a temporary array of sorted entries in a class field, but does not clean up the references set in this array after sorting.

This means that parts of the detached sub-Sceengraph survive garbage collection. The Texture memory counter just happens to be the most visible indicator...

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses critical memory leaks in JME3 where Spatial and Geometry objects were inadvertently retained in temporary arrays even after being detached from the scene graph. This retention prevented proper garbage collection, leading to a continuous increase in memory usage, particularly for texture memory. The changes ensure that these temporary arrays are explicitly cleared of references, allowing the garbage collector to reclaim memory associated with unreachable objects.

Highlights

• Prevented Spatial memory leaks: Explicitly nullified Spatial references in the temporary stack array used by Spatial#checkDoTransformUpdate() to ensure detached Spatial objects are garbage collected.
• Resolved ListSort memory retention: Cleared the temporary array (tmpArray) in ListSort#sort() after use, releasing references to Geometry objects and preventing their unintended retention.

Changelog

• jme3-core/src/main/java/com/jme3/scene/Spatial.java
  • Introduced explicit nullification of Spatial references in the temporary stack array within checkDoTransformUpdate() to prevent memory leaks, ensuring detached Spatial objects are properly garbage collected.
• jme3-core/src/main/java/com/jme3/util/ListSort.java
  • Added java.util.Arrays import.
  • Cleared the tmpArray used for merge sort in sort() method by filling it with null values. This prevents memory leaks by releasing references to Geometry objects that might otherwise be retained after sorting, allowing them to be garbage collected.

Activity

• No activity has been recorded yet for this pull request.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request effectively addresses two memory leaks related to object pooling and reused arrays. In Spatial.java, the change correctly clears references from a temporary stack and defers releasing the temporary variables object until it's no longer in use. In ListSort.java, the temporary array used for merge sort is now cleared, preventing stale references. Both changes are crucial for preventing memory leaks in long-running applications where scene graph elements are frequently attached and detached. I've also included a suggestion to fix a pre-existing off-by-one error in Spatial.java which could lead to a NullPointerException.

[riccardobl]

Good find! Since this is likely a bug that can potentially present itself everytime that TempVars field is used without proper cleanup, maybe we should fix it right into the TempVars.release method?

jmonkeyengine/jme3-core/src/main/java/com/jme3/util/TempVars.java

Lines 126 to 142 in f02c4e6

	public void release() {
	if (!isUsed) {
	throw new IllegalStateException("This instance of TempVars was already released!");
	}
	isUsed = false;
	TempVarsStack stack = varsLocal.get();
	// Return it to the stack
	stack.index--;
	// Check if it is actually there
	if (stack.tempVars[stack.index] != this) {
	throw new IllegalStateException("An instance of TempVars has not been released in a called method!");
	}
	}

we can call Arrays.fill from there, thoughts?

[parysto]

Yes, calling Arrays.fill(...) in TempVars would yield the same result and would probably be a lot cleaner, but it would always iterate over the stack even if it wasn't used by the releasing code. I think this causes a little performance impact we might want to avoid.