jbonofre commented on code in PR #1334: URL: https://github.com/apache/arrow-java/pull/1334#discussion_r4192653585
########## memory/memory-ffm/src/main/java/org/apache/arrow/memory/ffm/FfmAllocationManager.java: ########## @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.arrow.memory.ffm; + +import java.lang.foreign.Arena; +import java.lang.foreign.MemorySegment; +import org.apache.arrow.memory.AllocationManager; +import org.apache.arrow.memory.ArrowBuf; +import org.apache.arrow.memory.BufferAllocator; +import org.apache.arrow.memory.ReferenceManager; + +/** Allocation manager based on the Foreign Function & Memory API. */ +public final class FfmAllocationManager extends AllocationManager { + + private static final Arena EMPTY_ARENA = Arena.ofShared(); + private static final ArrowBuf EMPTY = + new ArrowBuf(ReferenceManager.NO_OP, null, 0, EMPTY_ARENA.allocate(0).address()); + + public static final AllocationManager.Factory FACTORY = + new Factory() { + @Override + public AllocationManager create(BufferAllocator accountingAllocator, long size) { + return new FfmAllocationManager(accountingAllocator, size); + } + + @Override + public ArrowBuf empty() { + return EMPTY; + } + }; + + private final Arena arena; + private final long allocatedSize; + private final long allocatedAddress; + + FfmAllocationManager(BufferAllocator accountingAllocator, long requestedSize) { + super(accountingAllocator); + this.arena = Arena.ofShared(); Review 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? ########## bom/pom.xml: ########## @@ -174,6 +174,11 @@ under the License. <artifactId>arrow-memory-core</artifactId> <version>${project.version}</version> </dependency> + <dependency> + <groupId>org.apache.arrow</groupId> + <artifactId>arrow-memory-ffm</artifactId> Review 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. ########## memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java: ########## @@ -16,149 +16,97 @@ */ package org.apache.arrow.memory.util; -import java.lang.reflect.Constructor; import java.lang.reflect.Field; -import java.lang.reflect.InaccessibleObjectException; -import java.lang.reflect.InvocationTargetException; import java.nio.ByteBuffer; import java.nio.ByteOrder; -import java.security.AccessController; -import java.security.PrivilegedAction; -import org.checkerframework.checker.nullness.qual.Nullable; -import sun.misc.Unsafe; +import org.apache.arrow.memory.DefaultAllocationManagerOption; +import org.apache.arrow.util.VisibleForTesting; /** Utilities for memory related operations. */ public class MemoryUtil { - private static final org.slf4j.Logger logger = - org.slf4j.LoggerFactory.getLogger(MemoryUtil.class); - - private static final @Nullable Constructor<?> DIRECT_BUFFER_CONSTRUCTOR; - - /** The unsafe object from which to access the off-heap memory. */ - private static final Unsafe UNSAFE; - - /** The start offset of array data relative to the start address of the array object. */ - private static final long BYTE_ARRAY_BASE_OFFSET; - - /** The offset of the address field with the {@link java.nio.ByteBuffer} object. */ - private static final long BYTE_BUFFER_ADDRESS_OFFSET; /** If the native byte order is little-endian. */ public static final boolean LITTLE_ENDIAN = ByteOrder.nativeOrder() == ByteOrder.LITTLE_ENDIAN; - // Java 1.8, 9, 11, 17, 21 becomes 1, 9, 11, 17, and 21. - @SuppressWarnings("StringSplitter") - private static final int majorVersion = - Integer.parseInt(System.getProperty("java.specification.version").split("\\D+")[0]); + /** + * The system property used to select the {@link MemoryUtilAccessor} implementation. When unset, + * this defaults to FFM if {@link + * DefaultAllocationManagerOption#ALLOCATION_MANAGER_TYPE_PROPERTY_NAME} (or its environment + * variable) is set to {@code FFM}, so that selecting the FFM allocation manager avoids {@code + * sun.misc.Unsafe} entirely unless this property overrides it. + */ + public static final String MEMORY_ACCESSOR_TYPE_PROPERTY_NAME = "arrow.memory.accessor.type"; - static { - try { - // try to get the unsafe object - final Object maybeUnsafe = - AccessController.doPrivileged( - new PrivilegedAction<Object>() { - @Override - @SuppressWarnings({"nullness:argument", "nullness:return"}) - // incompatible argument for parameter obj of Field.get - // incompatible types in return - public Object run() { - try { - final Field unsafeField = Unsafe.class.getDeclaredField("theUnsafe"); - unsafeField.setAccessible(true); - return unsafeField.get(null); - } catch (Throwable e) { - return e; - } - } - }); + private static final org.slf4j.Logger logger = + org.slf4j.LoggerFactory.getLogger(MemoryUtil.class); - if (maybeUnsafe instanceof Throwable) { - throw (Throwable) maybeUnsafe; - } + private static final MemoryUtilAccessor ACCESSOR = resolveAccessor(); - UNSAFE = (Unsafe) maybeUnsafe; + private MemoryUtil() {} - // get the offset of the data inside a byte array object - BYTE_ARRAY_BASE_OFFSET = UNSAFE.arrayBaseOffset(byte[].class); + /** Returns the fully qualified class name of the {@link MemoryUtilAccessor} in use. */ + @VisibleForTesting + public static String getAccessorClassName() { + return ACCESSOR.getClass().getName(); + } - // get the offset of the address field in a java.nio.Buffer object - long maybeOffset; - Field addressField = java.nio.Buffer.class.getDeclaredField("address"); + private static MemoryUtilAccessor resolveAccessor() { + String type = System.getProperty(MEMORY_ACCESSOR_TYPE_PROPERTY_NAME, ""); + if ("FFM".equals(type)) { + logger.info( + "{}=FFM, loading org.apache.arrow.memory.ffm.FfmMemoryAccessor", + MEMORY_ACCESSOR_TYPE_PROPERTY_NAME); + return loadFfmAccessor(); + } + if (type.isEmpty() + && DefaultAllocationManagerOption.getDefaultAllocationManagerType() + == DefaultAllocationManagerOption.AllocationManagerType.FFM) { try { - addressField.setAccessible(true); - maybeOffset = UNSAFE.objectFieldOffset(addressField); - } catch (InaccessibleObjectException e) { - maybeOffset = -1; - logger.debug( - "Cannot access the address field of java.nio.Buffer. DirectBuffer operations wont be available", + MemoryUtilAccessor accessor = loadFfmAccessor(); + logger.info( + "{}=FFM, also loading org.apache.arrow.memory.ffm.FfmMemoryAccessor to avoid" + + " sun.misc.Unsafe (override with {}=Unsafe if this is not wanted)", + DefaultAllocationManagerOption.ALLOCATION_MANAGER_TYPE_PROPERTY_NAME, + MEMORY_ACCESSOR_TYPE_PROPERTY_NAME); + return accessor; + } catch (RuntimeException e) { + // Unlike an explicit arrow.memory.accessor.type=FFM request, this preference is only Review 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 `Error`s, 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
