Memory allocators overhaul - #2835
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates dependency versions and integrates the SaferBufferAllocator dynamically across Android and iOS platforms. It also refactors AndroidNativeBufferAllocator and LibJGLIOSNativeBufferAllocator to automatically clean up native memory using phantom references and background threads, while simplifying and deprecating the concurrent LWJGL allocator. The review feedback suggests several robustness and performance improvements: throwing an explicit OutOfMemoryError when allocation fails in AndroidNativeBufferAllocator, wrapping the background deallocation loops in try-catch blocks to prevent silent thread termination, and using AtomicIntegerFieldUpdater instead of AtomicBoolean in the Deallocator classes to reduce garbage collection overhead.
|
🖼️ Screenshot tests have failed. The purpose of these tests is to ensure that changes introduced in this PR don't break visual features. They are visual unit tests. 📄 Where to find the report:
✅ If you did mean to change things: ✨ If you are creating entirely new tests: Note; it is very important that the committed reference images are created on the build pipeline, locally created images are not reliable. Similarly tests will fail locally but you can look at the report to check they are "visually similar". See https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-screenshot-tests/README.md for more information Contact @richardTingle (aka richtea) for guidance if required |
There was a problem hiding this comment.
Pull request overview
This PR overhauls direct buffer allocation across LWJGL3, Android, and iOS, adding safer allocator selection and PhantomReference-based native memory cleanup for mobile backends.
Changes:
- Adds GC-backed native buffer deallocation for Android and iOS allocators.
- Refactors LWJGL3 allocation bookkeeping to be thread-safe and deprecates the concurrent allocator alias.
- Adds saferalloc support/version updates and wires examples to include
jme3-saferallocator.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
jme3-lwjgl3/src/main/java/com/jme3/util/LWJGLBufferAllocator.java |
Refactors deallocator lifecycle with atomic free guards and deprecates the concurrent subclass. |
jme3-lwjgl3/src/main/java/com/jme3/system/lwjgl/LwjglContext.java |
Selects saferalloc when available and updates LWJGL allocator defaults. |
jme3-ios/src/main/java/com/jme3/util/LibJGLIOSNativeBufferAllocator.java |
Adds PhantomReference tracking and address-based native freeing. |
jme3-ios/src/main/java/com/jme3/system/ios/IGLESContext.java |
Prefers SaferBufferAllocator when present, otherwise uses the iOS native allocator. |
jme3-ios-examples/build.gradle |
Adds saferallocator dependency to iOS examples. |
jme3-core/src/main/java/com/jme3/util/ReflectionAllocator.java |
Marks reflection allocator as deprecated. |
jme3-android/src/main/java/com/jme3/util/AndroidNativeBufferAllocator.java |
Adds PhantomReference tracking and changes JNI methods to address-based freeing. |
jme3-android/src/main/java/com/jme3/app/AndroidHarnessFragment.java |
Selects saferalloc/native allocator before application creation. |
jme3-android/src/main/java/com/jme3/system/android/OGLESContext.java |
Selects saferalloc when available for Android context initialization. |
jme3-android-examples/build.gradle |
Adds saferallocator dependency to Android examples. |
gradle/libs.versions.toml |
Bumps libjglios/saferalloc and adds iOS saferalloc native artifact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
|
/gemini review again, i did not apply the AtomicIntegerFieldUpdater suggestion because it is a micro-optimization not worth the complexity |
There was a problem hiding this comment.
Code Review
This pull request introduces phantom-reference-based automatic deallocation for direct byte buffers in the Android and iOS native allocators to prevent native memory leaks. It also integrates the jme3-saferallocator module as a preferred allocator on Android and iOS when available, deprecates ReflectionAllocator and ConcurrentLWJGLBufferAllocator, and refactors LWJGLBufferAllocator to use AtomicBoolean for thread-safe deallocation. Feedback suggests using primitive long instead of boxed Long for the native memory address in LWJGLBufferAllocator to avoid unnecessary boxing overhead.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
* add memory management to ios and android allocators * default ios and android to saferalloc when available * make allocators fully thread safe * deprecate ReflectionAllocator * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <[email protected]> * update jmeAndroidNatives * Harden native buffer allocators against allocation and cleanup failures * Update jme3-lwjgl3/src/main/java/com/jme3/util/LWJGLBufferAllocator.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update jme3-lwjgl3/src/main/java/com/jme3/util/LWJGLBufferAllocator.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * update to saferalloc 0.0.10 * Fix saferalloc native memory pressure accounting * Add saferalloc memory pressure guard tests * make screenshot tests use safer allocator --------- Co-authored-by: Copilot Autofix powered by AI <[email protected]> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This PR improves memory allocators in every backend
jme3-saferallocatordependency is presentConcurrentLWJGLBufferAllocatorand turn it to an alias ofLWJGLBufferAllocator(since it is threadsafe now)ReflectionAllocatorthat is not supported in modern java nor guaranteed to work in non hostpot jvms