Thanks for sharing the information Laurent. I'd think JDK will always provide options to leverage garbage collector when it comes to cleaning up external resources (like native memory) binding to a object. In panama, as you can see, the new MemorySegment interfaces still allow users to register GC-hooks to do cleanup. See "Implicit deallocation" chapter in this documentation[1].
By the way, in Arrow things can be a little bit more complicated, since the allocations has a local scope which is BufferAllocator, but JDK always uses global direct memory scope, even in panama. Hongze [1] https://docs.oracle.com/en/java/javase/16/docs/api/jdk.incubator.foreign/jdk/incubator/foreign/MemorySegment.html On Thu, 2021-10-07 at 21:40 -0700, Laurent Goujon wrote: > Worth also observing that the Java community has been recognizant of the > issues with the current approach around direct byte buffer and GC, and has > come up with a proposal/implementation to avoid interaction with GC and > give more direct control. The proposal (actually, its 3rd iteration) is > described here at https://openjdk.java.net/jeps/393, and has been available > as an incubator feature for several JDK releases (Javadoc: > https://docs.oracle.com/en/java/javase/17/docs/api/jdk.incubator.foreign/jdk/incubator/foreign/package-summary.html > ) > > Laurent > > > On Thu, Oct 7, 2021 at 4:40 PM Jacques Nadeau <jacq...@apache.org> wrote: > > > Clearly this patch was driven by an implicit set of needs but it's hard to > > guess at what they are. As Laurent asked, what is the main goal here? There > > may be many ways to solve this goal. > > > > Some thoughts in general: > > - The allocator is a finely tuned piece of multithreaded machinery that is > > used on hundreds of thousands of vm's ever year. As such, refactors should > > be avoided without a clear pressing need. When a need exists, it should be > > discussed extensively and thoroughly designed/vetted before proposing any > > patches. > > - Having used the library extensively, I can state that the library is > > already too heap impacting given the nature of the objects. Increasing GC > > load/reliance is actually in direct conflict to many of the past > > optimizations of the allocator and vector objects. > > - As far as I can tell, a new allocation manager could do auto-releases. > > Why go beyond introducing a new allocation manager (or even a backstopping > > allocation manager facade). The allocation manager was actually built to > > solve for things like memory moving between C and java. > > - It isn't clear here how much this is proposed as a backstop mechanism > > versus a primary release mechanism. I'm more in support of an optional > > backstop mechanism so that production apps don't leak (with appropriate > > logged warnings, etc). > > - Relying on GC to release direct memory frequently leads to early OOMs > > (exactly why the release mechanism was in place and is chosen for high perf > > apps like Netty and Arrow). It is fairly easy to allocate a bunch of direct > > memory but the GC never kicks in because heap memory is not under load. > > > > Lastly, I agree with Micah on the library use confusion. If we have close() > > methods everywhere and they javadoc they are required to release memory but > > then we implement something that makes that possibly not true (or not true > > in some contexts), wowser on the confusion. > > > > > > > > > > On Thu, Oct 7, 2021 at 3:55 PM Micah Kornfield <emkornfi...@gmail.com> > > wrote: > > > > > In addition to the points raised by Laurent, I'm concerned about having > > two > > > possible idioms in the same library. It means code written against the > > > library becomes less portable (you need to know how the memory allocator > > is > > > using GC or not). > > > > > > I understand manual memory management in Java is tedious but is there a > > > specific problem this is addressing other than making Arrow have more > > > expected semantics to Java users? Are there maybe alternative solutions > > to > > > addressing the problem? > > > > > > > > > -Micah > > > > > > > > > > > > On Thu, Oct 7, 2021 at 3:48 PM Hongze Zhang <notify...@126.com> wrote: > > > > > > > We don't have to concern about that since no difference will be made on > > > > current manual release path unless "MemoryChunkCleaner" is explicitly > > > > specified when creating BufferAllocators. Manual ref counting will be > > > only > > > > discard/ignored in "MemoryChunkCleaner", while by default > > > > "MemoryChunkManager" is used which is totally the same as current. > > > > > > > > > > > > > > > > > > > > For example, to enable GC-based functionalities, the BufferAllocator > > has > > > > to be created using the following way: > > > > > > > > > > > > > > > > > > > > BufferAllocator root = new RootAllocator( > > > > > > > > BaseAllocator.configBuilder() > > > > > > > > .maxAllocation(MAX_ALLOCATION) > > > > > > > > .memoryChunkManagerFactory(MemoryChunkCleaner.newFactory()) > > > // > > > > to enable GC-based buffer release > > > > > > > > .listener(MemoryChunkCleaner.gcTrigger()) // to trigger JVM > > > GC > > > > when allocator is full > > > > > > > > .build()); > > > > > > > > Thanks, > > > > > > > > Hongze > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > At 2021-10-07 12:53:23, "Laurent Goujon" <laur...@dremio.com> wrote: > > > > > Unless I missed something in the big picture, but it seems that using > > a > > > GC > > > > > based approach would make things much inefficient in terms of memory > > > > > management and would cause probably quite the confusion for all the > > > > systems > > > > > out there which rely on refcounting for keeping things in check, I'm > > not > > > > > sure why changing the default is such a good idea... > > > > > > > > > > On Tue, Oct 5, 2021 at 2:20 AM Hongze Zhang <notify...@126.com> > > wrote: > > > > > > > > > > > Hi Laurent, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Sorry I might describe it unclearly and yes, the purpose is > > > > > > straightforward: to discard (in another strategy rather than > > default) > > > > the > > > > > > usage of Java buffer ref counting and rely on GC to do cleanup for > > the > > > > > > buffers. Though some design changes to Allocator APIs maybe required > > > to > > > > > > achieve this, for example, the decoupling of MemoryChunkAllocator > > from > > > > > > AllocationManager. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Should we make more discussion on it, or I can open a ticket asap? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > Hongze > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > At 2021-10-05 13:39:55, "Laurent Goujon" <laur...@dremio.com> > > wrote: > > > > > > > Hi, > > > > > > > > > > > > > > I gave a quick look at your patches but to be honest, it's not > > > > > > > easy > > > to > > > > > > > figure out the problem you're trying to solve in the first place. > > Or > > > > is it > > > > > > > simply to discard the use of ref counting to track buffer usages > > and > > > > > > > relying on Java references to keep track of buffers at the expense > > of > > > > > > > creating more pressure on the GC itself to collect and free > > buffers? > > > > > > > > > > > > > > On Wed, Sep 29, 2021 at 11:58 PM Hongze Zhang <notify...@126.com> > > > > wrote: > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > I would like to discuss on the potential of introducing a > > GC-based > > > > > > > > reference management strategy to Arrow Java, and we > > > > > > > > have already been working on an implementation in our own > > project. > > > I > > > > > > have > > > > > > > > put the related codes in following branch and > > > > > > > > if it makes sense to upstream Apache Arrow I can open a PR for > > it: > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/zhztheplayer/arrow-1/commits/wip-chunk-cleaner > > > > > > > > > > > > > > > > In the branch, regarding Commit 1, Commit 2 and Commit 3: > > > > > > > > > > > > > > > > Commit 1: To break AllocationManager to two components: > > > > > > > > MemoryChunkManager[1] which maintains BufferLedger- > > > > > > > > BufferAllocator mappings, and MemoryChunkAllocator[2] which > > > performs > > > > the > > > > > > > > underlying allocate/destory operations. The > > > > > > > > previous customizations such as NettyAllocationManager, > > > > > > > > UnsafeAllocationManager, are moved to MemoryChunkAllocator API, > > > > > > > > as NettyMemoryChunkAllocator and UnsafeMemoryChunkAllocator. > > > > > > > > > > > > > > > > Commit 2: To introduce a new implementation of > > MemoryChunkManager, > > > > > > > > MemoryChunkCleaner[3]. By default, MemoryChunkCleaner > > > > > > > > converts all managed chunks to WeakReferences, and a global > > thread > > > > will > > > > > > > > observe on the assigned reference queue to > > > > > > > > release the unused garbage chunks which are enqueued by JVM GC. > > > Some > > > > > > > > modes[4] are there to provide different strategies, > > > > > > > > such as to disable manual reference management (default), or to > > > > enable > > > > > > > > manual and GC-based reference management at the > > > > > > > > same time, or to report leaks only. > > > > > > > > > > > > > > > > Commit 3: Add API "ArrowBuf buffer(MemoryChunk chunk)" to > > > > > > BufferAllocator. > > > > > > > > This makes some special workloads, e.g. > > > > > > > > cross-JNI buffer sharing much easier as we will no longer need > > > > > > > > an > > > > > > > > Allocator directly binding to the shared memory > > > > > > > > chunks, and will still be able to leverage all of Allocator's > > > > advantages > > > > > > > > to manage the chunks (E.g. to use > > > > > > > > MemoryChunkCleaner). > > > > > > > > > > > > > > > > Some possible Q&As about this implementation: > > > > > > > > > > > > > > > > 1. Why adding MemoryChunkAllocator, rather than just customizing > > > > > > > > AllocationManager? > > > > > > > > It is to reuse all of current underlying chunk allocation > > > strategies, > > > > > > > > Netty, Unsafe, and others. Also, with the layer of > > > > > > > > MemoryChunk separated from AllocationManager we will be able to > > > > create > > > > > > > > MemoryChunks actively in some special cases, e.g. > > > > > > > > cross-JNI buffer sharing, C data interface buffer importing, > > > > > > > > then > > > > > > populate > > > > > > > > the chunks to a manager BufferAllocator. > > > > > > > > > > > > > > > > 2. Why using WeakReference rather than PhantomReference? > > > > > > > > In Java 8, PhantomReference has some issue against its referent > > > > object. > > > > > > > > The object cannot be garbage collected before > > > > > > > > being enqueued. In WeakReference we don't have this issue. See > > > > ref[5]. > > > > > > > > > > > > > > > > 3. Why not sun.misc.Cleaner? > > > > > > > > Cleaner API maintains a global doubly linked list to keep the > > > Cleaner > > > > > > > > instances alive. This brings overheads to us since > > > > > > > > we will create local doubly linked list for cleaning up all the > > > > buffers > > > > > > on > > > > > > > > Allocator close. See a unit test[6]. > > > > > > > > > > > > > > > > 4. Why closing all buffers on Allocator close? > > > > > > > > This behavior can be customizabale within Mode[7]s. A principle > > is, > > > > when > > > > > > > > relying on GC, we should allow closing buffers > > > > > > > > manually, or at least closing all of them on Allocator close. Or > > > the > > > > > > > > actual release time of the underlying chunks will > > > > > > > > be unpredictable. > > > > > > > > > > > > > > > > 5. Can we use heap based buffers? > > > > > > > > If I am not wrong, no. The heap objects can be physically moved > > > > around > > > > > > by > > > > > > > > JVM. The addresses can vary. > > > > > > > > > > > > > > > > 6. When should GC happen? > > > > > > > > A new AllocationListener implementation, GCTriger[8] is > > introduced. > > > > GC > > > > > > > > will be performed when BufferAllocator is full. > > > > > > > > > > > > > > > > 7. Performance? > > > > > > > > Based on previous measurement, it doesn't bring overheads on the > > > > legacy > > > > > > > > path (by using default MemoryChunkManager). For > > > > > > > > GC-based path (MemoryChunkCleaner + Mode.GC_ONLY), allocations > > can > > > be > > > > > > > > comparatively slower due to higher GC pressure > > > > > > > > (For example, in out Arrow-based query engine, to run TPC-DS > > > SF1000, > > > > the > > > > > > > > overhead can be up to 3%). I can collect more > > > > > > > > benchmark results in future, maybe under a JIRA ticket or a PR. > > > > > > > > > > > > > > > > 8. Breaking changes? > > > > > > > > It doesn't break library-based allocation strategy selection > > > > > > > > like > > > > > > directly > > > > > > > > including arrow-memory-unsafe.jar to > > > > > > > > projects. However it breaks use of the previous > > > > > > AllocationManager.Factory > > > > > > > > API. Users should migrate to > > > > > > > > MemoryChunkAllocator API. Which in my opinion is simple to do. > > > > > > > > > > > > > > > > The implementation is still evolving, so if the links get > > > out-of-date > > > > > > you > > > > > > > > can check on the newest branch head. Any > > > > > > > > suggestions welcome. > > > > > > > > > > > > > > > > Some other discussions that may be related to this topic: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://lists.apache.org/thread.html/r09b2e7dd8e342818190c332ee20d97f68816241d0c683e17c4f35dc6%40%3Cdev.arrow.apache.org%3E > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://lists.apache.org/thread.html/rc956b16b7ed9768cce8eaecafdc363f7b3808dafe6edf1940b139ecb%40%3Cuser.arrow.apache.org%3E > > > > > > > > > > > > > > > > Best, > > > > > > > > Hongze > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > https://github.com/zhztheplayer/arrow-1/blob/52e5def7a239164b5c13457c8be38e227bb2907d/java/memory/memory-core/src/main/java/org/apache/arrow/memory/ > > > > > > > > MemoryChunkManager.java > > > > > > > > [2] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/zhztheplayer/arrow-1/blob/52e5def7a239164b5c13457c8be38e227bb2907d/java/memory/memory-core/src/main/java/org/apache/arrow/memory/MemoryChunkAllocator.java > > > > > > > > > > > > > > > > [3] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/zhztheplayer/arrow-1/blob/52e5def7a239164b5c13457c8be38e227bb2907d/java/memory/memory-core/src/main/java/org/apache/arrow/memory/MemoryChunkCleaner.java > > > > > > > > [4] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/zhztheplayer/arrow-1/blob/52e5def7a239164b5c13457c8be38e227bb2907d/java/memory/memory-core/src/main/java/org/apache/arrow/memory/MemoryChunkCleaner.java#L503 > > > > > > > > [5] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://stackoverflow.com/questions/56684150/why-since-java-9-phantomreference-java-doc-states-that-it-is-dedicated-to-the-po > > > > > > > > [6] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/zhztheplayer/arrow-1/blob/52e5def7a239164b5c13457c8be38e227bb2907d/java/memory/memory-core/src/test/java/org/apache/arrow/memory/TestMemoryChunkCleaner.java#L204 > > > > > > > > [7] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/zhztheplayer/arrow-1/blob/52e5def7a239164b5c13457c8be38e227bb2907d/java/memory/memory-core/src/main/java/org/apache/arrow/memory/MemoryChunkCleaner.java#L503 > > > > > > > > [8] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/zhztheplayer/arrow-1/blob/52e5def7a239164b5c13457c8be38e227bb2907d/java/memory/memory-core/src/main/java/org/apache/arrow/memory/MemoryChunkCleaner.java#L402 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
