Repository navigation
Conversation
- Move the sun.misc.Unsafe code from MemoryUtil into UnsafeMemoryAccessor - MemoryUtil delegates every low-level operation to a MemoryUtilAccessor - No behavior change: UnsafeMemoryAccessor is the only accessor
- New opt-in module (JDK 22+, java.lang.foreign per JEP 454), only part of the Maven reactor when building with a JDK 22+ launcher - FfmMemoryAccessor implements MemoryUtilAccessor with MemorySegment and Arena instead of sun.misc.Unsafe and reflection - FfmAllocationManager allocates one Arena per buffer, mirroring UnsafeAllocationManager, with a DefaultAllocationManagerFactory for CheckAllocator's classpath scan - arrow.memory.accessor.type=FFM selects the FFM accessor and fails with an actionable message if arrow-memory-ffm is missing; unknown values warn and fall back to Unsafe - arrow.allocation.manager.type=FFM selects FfmAllocationManager and, when arrow.memory.accessor.type is unset, the FFM accessor too, falling back to Unsafe with a warning if the module is missing - Isolated Surefire executions cover each property combination, with and without add-opens - Add the module to the BOM and to the install and overview docs
fb64
changed the title
|
Thank you for opening a pull request! Please label the PR with one or more of:
Also, add the 'breaking-change' label if appropriate. See CONTRIBUTING.md for details. |
| MEMORY_ACCESSOR_TYPE_PROPERTY_NAME); | ||
| return accessor; | ||
| } catch (RuntimeException e) { | ||
| // Unlike an explicit arrow.memory.accessor.type=FFM request, this preference is only |
There was a problem hiding this comment.
The fallback to Unsafe only catches RuntimeException, and loadFfmAccessor() only converts ReflectiveOperationException . Suppose -Darrow.allocation.manager.type=FFM is set on JDK 21 or earlier with the arrow-memory-ffm jar on the classpath. Loading FfmMemoryAccessor would throw UnsupportedClassVersionError, since the module is compiled for release 22. An ExceptionInInitializerError from its static init would do the same. Both are Errors, so they skip this fallback and MemoryUtil.<clinit> fails. The class then stays unusable for the life of the JVM, which is the situation the comment in this catch block says the fallback avoids.
Could we catch RuntimeException | LinkageError here? A test that forces the FFM path on a pre-22 JDK would also help.
| </dependency> | ||
| <dependency> | ||
| <groupId>org.apache.arrow</groupId> | ||
| <artifactId>arrow-memory-ffm</artifactId> |
There was a problem hiding this comment.
The BOM lists arrow-memory-ffm unconditionally, but memory/pom.xml only builds the memory-ffm module under the arrow-memory-ffm profile, which is active on [22,). If a release or deploy is built on JDK 17 or 21, the published BOM will reference an artifact that was never built. Anyone who imports the BOM and adds arrow-memory-ffm would then get an unresolvable dependency.
It's not a big deal, but as arrow-java release is not cut with JDK 22+ (for now), I wanted to mention that.
There was a problem hiding this comment.
Good point, indeed. So what are the options:
- Build the release job on JDK 25 and target Java 17 for other modules
maven.compiler.release=17 - Remove the
arrow-memory-ffmfrom the BOM until releases are built on JDK 22+.
Let me know which one fit the best for now ...
|
|
||
| FfmAllocationManager(BufferAllocator accountingAllocator, long requestedSize) { | ||
| super(accountingAllocator); | ||
| this.arena = Arena.ofShared(); |
There was a problem hiding this comment.
Nit: each allocation creates its own Arena.ofShared() here, and release0() calls arena.close(). Closing a shared arena requires a handshake will all threads. Workloads that allocate and free many small buffers, such as vector resizing or per-batch Flight allocations, will likely be much slower than with the Unsafe and Netty managers. This doesn't need to block the PR. It might be worth documenting as a known limitation, or following up with a different arena strategy. Do we have any benchmark numbers?
There was a problem hiding this comment.
Thanks, nice catch @jbonofre, you were right! Benchmark and results: https://gist.github.com/fb64/334d2eeb14e1e4c5adbff6e38e9f3b7e
Using one shared Arena per buffer makes allocation about 100x slower than Unsafe, and it gets worse with more JVM threads (4 KiB: Unsafe 89 ns vs FFM 8.8 µs, and 58.8 µs with 256 idle threads).
I also prototyped a version that allocates with malloc/free downcalls and keeps the same FFM accessor: it matches Unsafe on every workload, including vector growth and batch building. So memory access isn't the issue, only the per-buffer arena.
This doesn't weaken safety: the FFM accessor already accesses memory through new global segments built from the buffer's raw address, not through the arena's segment, so closing the arena never protected against use-after-free. Arrow's protection comes from ArrowBuf's reference count checks, which don't change. Unlike Arena.allocate, malloc also doesn't zero the memory, which matches the Unsafe and Netty managers.
I propose to update this PR with a malloc/free implementation based on the foreign linker instead of one shared Arena per buffer. Does that sound good?
There was a problem hiding this comment.
@fb64 yes, agree for a malloc/free impl in this PR. Happy to help and review 😄
There was a problem hiding this comment.
✅ I also updated the benchmark results in the gist after multiple run on my laptop (Mac book pro M1)
[fix] fall back to unsafe on ffm linkage errors - An inferred FFM accessor (arrow.allocation.manager.type=FFM) now also falls back to Unsafe on LinkageError: UnsupportedClassVersionError on JDK 21 or earlier, or a failing FfmMemoryAccessor static initializer - resolveAccessor takes its inputs as parameters, so tests can simulate these errors without a pre-22 JDK - Addresses review comment r4192588927
[fix] init ffm allocation factory before empty buffer - Declare FfmAllocationManager.FACTORY before EMPTY: creating EMPTY can initialize BaseAllocator, whose default config reads FACTORY back while the class is still initializing, and got null - With only arrow-memory-ffm on the classpath, accessing FACTORY first failed with an NPE in BaseAllocator's static initializer - Add an isolated Surefire execution, since the test needs a fresh JVM
[fix] allocate ffm buffers with malloc and free - Replace one shared Arena per buffer with malloc/free downcalls through the foreign linker, in FfmAllocationManager and FfmMemoryAccessor - Closing a shared Arena handshakes with every JVM thread: allocating and releasing 4 KiB drops from 8.8 us to 97 ns (Unsafe: 90 ns), and from 58.8 us to 99 ns with 256 idle threads - Memory is no longer zeroed on allocation, like the Unsafe and Netty managers - FfmMemoryAccessor.freeMemory now frees any malloc address, which removes the address-to-arena map and its silent no-op for foreign addresses - A failed malloc throws OutOfMemoryError, as Arena.allocate and Unsafe do - Addresses review comment r4192653585
2 participants
What's Changed
MemoryUtilrelies onsun.misc.Unsafewhichever allocation manager is used, and on reflection that requires--add-opens=java.base/java.nio=ALL-UNNAMED. Unsafe memory access is deprecated for removal (JEP 471) and has printed a warning since JDK 24 (JEP 498). This PR adds an opt-in alternative built on the FFM API, which is final since JDK 22 (JEP 454).MemoryUtilmoves toUnsafeMemoryAccessor, behind a newMemoryUtilAccessorinterface. Unsafe stays the default.arrow-memory-ffmmodule (JDK 22+):FfmMemoryAccessorusesMemorySegment/Arenainstead of Unsafe and reflection, andFfmAllocationManagerallocates oneArenaper buffer. The module is only built with a JDK 22+ launcher. It's added to the BOM and the docs.-Darrow.allocation.manager.type=FFMswitches both the allocator and the accessor, so Unsafe isn't used at all.-Darrow.memory.accessor.type=FFMswitches only the accessor.With FFM,
--add-opensis no longer needed, and a dedicated test run checks that.MemorySegment.reinterpretis a restricted method, so pass--enable-native-access=org.apache.arrow.memory.ffm(orALL-UNNAMEDon the classpath) to avoid a JVM warning.I assume this request was created with the help of AI agent (Claude).
Closes #163.