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 > > >> >> > > >> >> > > >> >> > > >> > > >
