xintongsong commented on a change in pull request #9747: [FLINK-13985] Use unsafe memory for managed memory URL: https://github.com/apache/flink/pull/9747#discussion_r335273631
########## File path: flink-core/src/main/java/org/apache/flink/core/memory/MemoryUtils.java ########## @@ -57,4 +64,79 @@ /** Should not be instantiated. */ private MemoryUtils() {} + + private static Constructor<? extends ByteBuffer> getDirectBufferPrivateConstructor() { + //noinspection OverlyBroadCatchBlock + try { + Constructor<? extends ByteBuffer> constructor = + ByteBuffer.allocateDirect(1).getClass().getDeclaredConstructor(long.class, int.class); + constructor.setAccessible(true); + return constructor; + } catch (NoSuchMethodException e) { + ExceptionUtils.rethrow( + e, + "The private constructor java.nio.DirectByteBuffer.<init>(long, int) is not available."); + } catch (SecurityException e) { + ExceptionUtils.rethrow( + e, + "The private constructor java.nio.DirectByteBuffer.<init>(long, int) is not available, " + + "permission denied by security manager"); + } catch (Throwable t) { + ExceptionUtils.rethrow( + t, + "Unclassified error while trying to access private constructor " + + "java.nio.DirectByteBuffer.<init>(long, int)."); + } + throw new RuntimeException("unexpected to avoid returning null"); + } + + /** + * Allocates unsafe native memory. + * + * @param size size of the unsafe memory to allocate. + * @return address of the allocated unsafe memory + */ + static long allocateUnsafe(long size) { + return UNSAFE.allocateMemory(Math.max(1L, size)); + } + + /** + * Creates a cleaner to release the unsafe memory by VM GC. + * + * <p>When memory owner becomes <a href="package-summary.html#reachability">phantom reachable</a>, + * GC will release the underlying unsafe memory if not released yet. + * + * @param owner memory owner which phantom reaching is to monitor by GC and release the unsafe memory + * @param address address of the unsafe memory to release + * @return action to run to release the unsafe memory manually + */ + static Runnable createMemoryGcCleaner(@Nullable Object owner, long address) { + return createGcCleaner(owner, () -> releaseUnsafe(address)); + } + + @SuppressWarnings("UseOfSunClasses") + private static Runnable createGcCleaner(@Nullable Object owner, Runnable toClean) { + return owner == null ? toClean : sun.misc.Cleaner.create(owner, toClean)::clean; Review comment: Agree with removing null case. I get the idea that we use `Cleaner#clean` to guarantee that `Unsafe#freeMemory` will be called only once. And I think it's a elegant design. My point is we should add test cases to protect this. E.g., first call `HybridMemorySegment#free` then explicitly trigger a GC, and verify that there should be no failures due to multiple calls of `Unsafe#freeMemory`. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services