From 1784f92209c2c5acfc8be526cdde1b7972a1378f Mon Sep 17 00:00:00 2001 From: Rong Rong Date: Tue, 14 Jan 2020 18:03:09 -0800 Subject: [PATCH 1/5] fix requested and allocated size difference --- .../apache/arrow/memory/AllocationManager.java | 18 +++++++++++------- .../org/apache/arrow/memory/BaseAllocator.java | 4 ++-- .../org/apache/arrow/memory/BufferLedger.java | 8 ++++---- .../arrow/memory/NettyAllocationManager.java | 2 +- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java b/java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java index e277ec0da90..1f67508c82e 100644 --- a/java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java +++ b/java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java @@ -53,7 +53,7 @@ public abstract class AllocationManager { // see JIRA for details private final LowCostIdentityHashMap map = new LowCostIdentityHashMap<>(); private final long amCreationTime = System.nanoTime(); - private final long size; + private final long requestedSize; // The ReferenceManager created at the time of creation of this AllocationManager // is treated as the owning reference manager for the underlying chunk of memory @@ -61,11 +61,11 @@ public abstract class AllocationManager { private volatile BufferLedger owningLedger; private volatile long amDestructionTime = 0; - protected AllocationManager(BaseAllocator accountingAllocator, long size) { + protected AllocationManager(BaseAllocator accountingAllocator, long requestedSize) { Preconditions.checkNotNull(accountingAllocator); accountingAllocator.assertOpen(); - this.size = size; + this.requestedSize = requestedSize; this.root = accountingAllocator.root; // we do a no retain association since our creator will want to retrieve the newly created @@ -154,10 +154,10 @@ void release(final BufferLedger ledger) { // the only mapping was for the owner // which now has been removed, it implies we can safely destroy the // underlying memory chunk as it is no longer being referenced - ((BaseAllocator)oldLedger.getAllocator()).releaseBytes(getSize()); + ((BaseAllocator)oldLedger.getAllocator()).releaseBytes(getAllocatedSize()); // free the memory chunk associated with the allocation manager release0(); - ((BaseAllocator)oldLedger.getAllocator()).getListener().onRelease(getSize()); + ((BaseAllocator)oldLedger.getAllocator()).getListener().onRelease(getAllocatedSize()); amDestructionTime = System.nanoTime(); owningLedger = null; } else { @@ -181,10 +181,14 @@ void release(final BufferLedger ledger) { /** * Return the size of underlying chunk of memory managed by this Allocation Manager. * + *

By default the requested size is the actual size of the memory chunk. + * However if concrete AllocationManager impl supports allocating different bytes of memory + * comparing to desired, it must also override the getSize() method; * @return size of underlying memory chunk */ - public long getSize() { - return size; + public long getAllocatedSize() { + // by default returns the requested size, this is the default AllcationManager behavior. + return requestedSize; } /** diff --git a/java/memory/src/main/java/org/apache/arrow/memory/BaseAllocator.java b/java/memory/src/main/java/org/apache/arrow/memory/BaseAllocator.java index 55144ae39fc..d8bbef87b74 100644 --- a/java/memory/src/main/java/org/apache/arrow/memory/BaseAllocator.java +++ b/java/memory/src/main/java/org/apache/arrow/memory/BaseAllocator.java @@ -606,7 +606,7 @@ private void verifyAllocator( } buffersSeen.put(am, this); - bufferTotal += am.getSize(); + bufferTotal += am.getAllocatedSize(); } // Preallocated space has to be accounted for @@ -722,7 +722,7 @@ private void dumpBuffers(final StringBuilder sb, final Set ledgerS sb.append("UnsafeDirectLittleEndian[identityHashCode == "); sb.append(Integer.toString(System.identityHashCode(am))); sb.append("] size "); - sb.append(Long.toString(am.getSize())); + sb.append(Long.toString(am.getAllocatedSize())); sb.append('\n'); } } diff --git a/java/memory/src/main/java/org/apache/arrow/memory/BufferLedger.java b/java/memory/src/main/java/org/apache/arrow/memory/BufferLedger.java index c94811e2996..75800e8f3b0 100644 --- a/java/memory/src/main/java/org/apache/arrow/memory/BufferLedger.java +++ b/java/memory/src/main/java/org/apache/arrow/memory/BufferLedger.java @@ -371,8 +371,8 @@ boolean transferBalance(final ReferenceManager targetReferenceManager) { targetReferenceManager.getAllocator().getName()); } - boolean overlimit = targetAllocator.forceAllocate(allocationManager.getSize()); - allocator.releaseBytes(allocationManager.getSize()); + boolean overlimit = targetAllocator.forceAllocate(allocationManager.getAllocatedSize()); + allocator.releaseBytes(allocationManager.getAllocatedSize()); // since the transfer can only happen from the owning reference manager, // we need to set the target ref manager as the new owning ref manager // for the chunk of memory in allocation manager @@ -464,7 +464,7 @@ public boolean getAllocationFit() { */ @Override public long getSize() { - return allocationManager.getSize(); + return allocationManager.getAllocatedSize(); } /** @@ -477,7 +477,7 @@ public long getSize() { public long getAccountedSize() { synchronized (allocationManager) { if (allocationManager.getOwningLedger() == this) { - return allocationManager.getSize(); + return allocationManager.getAllocatedSize(); } else { return 0; } diff --git a/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java b/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java index 1a3824162d3..e1da5f3391a 100644 --- a/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java +++ b/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java @@ -62,7 +62,7 @@ protected void release0() { } @Override - public long getSize() { + public long getAllocatedSize() { return size; } From 489c2036de83fb823f6c97726cf152920e559921 Mon Sep 17 00:00:00 2001 From: Rong Rong Date: Tue, 14 Jan 2020 18:44:04 -0800 Subject: [PATCH 2/5] fix checkstyle --- .../arrow/memory/TestBaseAllocator.java | 58 ++++++++++--------- 1 file changed, 32 insertions(+), 26 deletions(-) diff --git a/java/memory/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java b/java/memory/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java index f01ff653b10..e73a5516f76 100644 --- a/java/memory/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java +++ b/java/memory/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java @@ -391,36 +391,42 @@ public void testCustomizedAllocationManager() { private BaseAllocator createAllocatorWithCustomizedAllocationManager() { return new RootAllocator(BaseAllocator.configBuilder() .maxAllocation(MAX_ALLOCATION) - .allocationManagerFactory((accountingAllocator, size) -> new AllocationManager(accountingAllocator, size) { - private final Unsafe unsafe = getUnsafe(); - private final long address = unsafe.allocateMemory(size); + .allocationManagerFactory((accountingAllocator, requestedSize) -> + new AllocationManager(accountingAllocator, requestedSize) { + private final Unsafe unsafe = getUnsafe(); + private final long address = unsafe.allocateMemory(requestedSize); + + @Override + protected long memoryAddress() { + return address; + } - @Override - protected long memoryAddress() { - return address; - } + @Override + protected void release0() { + unsafe.setMemory(address, requestedSize, (byte) 0); + unsafe.freeMemory(address); + } - @Override - protected void release0() { - unsafe.setMemory(address, size, (byte) 0); - unsafe.freeMemory(address); - } + @Override + public long getAllocatedSize() { + return requestedSize; + } - private Unsafe getUnsafe() { - Field f = null; - try { - f = Unsafe.class.getDeclaredField("theUnsafe"); - f.setAccessible(true); - return (Unsafe) f.get(null); - } catch (NoSuchFieldException | IllegalAccessException e) { - throw new RuntimeException(e); - } finally { - if (f != null) { - f.setAccessible(false); + private Unsafe getUnsafe() { + Field f = null; + try { + f = Unsafe.class.getDeclaredField("theUnsafe"); + f.setAccessible(true); + return (Unsafe) f.get(null); + } catch (NoSuchFieldException | IllegalAccessException e) { + throw new RuntimeException(e); + } finally { + if (f != null) { + f.setAccessible(false); + } + } } - } - } - }).build()); + }).build()); } // Allocation listener From 97e7d3b8a593c70a8b79eace516cce7ff9bf2edf Mon Sep 17 00:00:00 2001 From: Rong Rong Date: Wed, 15 Jan 2020 15:04:24 -0800 Subject: [PATCH 3/5] address patch comments --- .../arrow/memory/AllocationManager.java | 18 ++---- .../apache/arrow/memory/BaseAllocator.java | 4 +- .../org/apache/arrow/memory/BufferLedger.java | 8 +-- .../arrow/memory/NettyAllocationManager.java | 19 +++--- .../arrow/memory/TestBaseAllocator.java | 63 +++++++++---------- 5 files changed, 55 insertions(+), 57 deletions(-) diff --git a/java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java b/java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java index 1f67508c82e..a6ccde55c91 100644 --- a/java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java +++ b/java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java @@ -53,7 +53,6 @@ public abstract class AllocationManager { // see JIRA for details private final LowCostIdentityHashMap map = new LowCostIdentityHashMap<>(); private final long amCreationTime = System.nanoTime(); - private final long requestedSize; // The ReferenceManager created at the time of creation of this AllocationManager // is treated as the owning reference manager for the underlying chunk of memory @@ -61,11 +60,10 @@ public abstract class AllocationManager { private volatile BufferLedger owningLedger; private volatile long amDestructionTime = 0; - protected AllocationManager(BaseAllocator accountingAllocator, long requestedSize) { + protected AllocationManager(BaseAllocator accountingAllocator) { Preconditions.checkNotNull(accountingAllocator); accountingAllocator.assertOpen(); - this.requestedSize = requestedSize; this.root = accountingAllocator.root; // we do a no retain association since our creator will want to retrieve the newly created @@ -154,10 +152,10 @@ void release(final BufferLedger ledger) { // the only mapping was for the owner // which now has been removed, it implies we can safely destroy the // underlying memory chunk as it is no longer being referenced - ((BaseAllocator)oldLedger.getAllocator()).releaseBytes(getAllocatedSize()); + ((BaseAllocator)oldLedger.getAllocator()).releaseBytes(getSize()); // free the memory chunk associated with the allocation manager release0(); - ((BaseAllocator)oldLedger.getAllocator()).getListener().onRelease(getAllocatedSize()); + ((BaseAllocator)oldLedger.getAllocator()).getListener().onRelease(getSize()); amDestructionTime = System.nanoTime(); owningLedger = null; } else { @@ -181,15 +179,11 @@ void release(final BufferLedger ledger) { /** * Return the size of underlying chunk of memory managed by this Allocation Manager. * - *

By default the requested size is the actual size of the memory chunk. - * However if concrete AllocationManager impl supports allocating different bytes of memory - * comparing to desired, it must also override the getSize() method; + *

The underlying memory chunk managed can be different from the original requested size. + * * @return size of underlying memory chunk */ - public long getAllocatedSize() { - // by default returns the requested size, this is the default AllcationManager behavior. - return requestedSize; - } + protected abstract long getSize(); /** * Return the absolute memory address pointing to the fist byte of underling memory chunk. diff --git a/java/memory/src/main/java/org/apache/arrow/memory/BaseAllocator.java b/java/memory/src/main/java/org/apache/arrow/memory/BaseAllocator.java index d8bbef87b74..55144ae39fc 100644 --- a/java/memory/src/main/java/org/apache/arrow/memory/BaseAllocator.java +++ b/java/memory/src/main/java/org/apache/arrow/memory/BaseAllocator.java @@ -606,7 +606,7 @@ private void verifyAllocator( } buffersSeen.put(am, this); - bufferTotal += am.getAllocatedSize(); + bufferTotal += am.getSize(); } // Preallocated space has to be accounted for @@ -722,7 +722,7 @@ private void dumpBuffers(final StringBuilder sb, final Set ledgerS sb.append("UnsafeDirectLittleEndian[identityHashCode == "); sb.append(Integer.toString(System.identityHashCode(am))); sb.append("] size "); - sb.append(Long.toString(am.getAllocatedSize())); + sb.append(Long.toString(am.getSize())); sb.append('\n'); } } diff --git a/java/memory/src/main/java/org/apache/arrow/memory/BufferLedger.java b/java/memory/src/main/java/org/apache/arrow/memory/BufferLedger.java index 75800e8f3b0..c94811e2996 100644 --- a/java/memory/src/main/java/org/apache/arrow/memory/BufferLedger.java +++ b/java/memory/src/main/java/org/apache/arrow/memory/BufferLedger.java @@ -371,8 +371,8 @@ boolean transferBalance(final ReferenceManager targetReferenceManager) { targetReferenceManager.getAllocator().getName()); } - boolean overlimit = targetAllocator.forceAllocate(allocationManager.getAllocatedSize()); - allocator.releaseBytes(allocationManager.getAllocatedSize()); + boolean overlimit = targetAllocator.forceAllocate(allocationManager.getSize()); + allocator.releaseBytes(allocationManager.getSize()); // since the transfer can only happen from the owning reference manager, // we need to set the target ref manager as the new owning ref manager // for the chunk of memory in allocation manager @@ -464,7 +464,7 @@ public boolean getAllocationFit() { */ @Override public long getSize() { - return allocationManager.getAllocatedSize(); + return allocationManager.getSize(); } /** @@ -477,7 +477,7 @@ public long getSize() { public long getAccountedSize() { synchronized (allocationManager) { if (allocationManager.getOwningLedger() == this) { - return allocationManager.getAllocatedSize(); + return allocationManager.getSize(); } else { return 0; } diff --git a/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java b/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java index e1da5f3391a..66dcd519caa 100644 --- a/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java +++ b/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java @@ -34,13 +34,13 @@ public class NettyAllocationManager extends AllocationManager { static final UnsafeDirectLittleEndian EMPTY = INNER_ALLOCATOR.empty; static final long CHUNK_SIZE = INNER_ALLOCATOR.getChunkSize(); - private final int size; + private final int allocatedSize; private final UnsafeDirectLittleEndian memoryChunk; - NettyAllocationManager(BaseAllocator accountingAllocator, int size) { - super(accountingAllocator, size); - this.memoryChunk = INNER_ALLOCATOR.allocate(size); - this.size = memoryChunk.capacity(); + NettyAllocationManager(BaseAllocator accountingAllocator, int requestedSize) { + super(accountingAllocator); + this.memoryChunk = INNER_ALLOCATOR.allocate(requestedSize); + this.allocatedSize = memoryChunk.capacity(); } /** @@ -61,9 +61,14 @@ protected void release0() { memoryChunk.release(); } + /** + * Returns the underlying memory chunk size managed. + * + *

NettyAllocationManager manages memory chunk by the exact requested size. + */ @Override - public long getAllocatedSize() { - return size; + protected long getSize() { + return allocatedSize; } /** diff --git a/java/memory/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java b/java/memory/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java index e73a5516f76..2be96c0c506 100644 --- a/java/memory/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java +++ b/java/memory/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java @@ -391,42 +391,41 @@ public void testCustomizedAllocationManager() { private BaseAllocator createAllocatorWithCustomizedAllocationManager() { return new RootAllocator(BaseAllocator.configBuilder() .maxAllocation(MAX_ALLOCATION) - .allocationManagerFactory((accountingAllocator, requestedSize) -> - new AllocationManager(accountingAllocator, requestedSize) { - private final Unsafe unsafe = getUnsafe(); - private final long address = unsafe.allocateMemory(requestedSize); - - @Override - protected long memoryAddress() { - return address; - } + .allocationManagerFactory((accountingAllocator, requestedSize) -> new AllocationManager(accountingAllocator) { + private final Unsafe unsafe = getUnsafe(); + private final long address = unsafe.allocateMemory(requestedSize); - @Override - protected void release0() { - unsafe.setMemory(address, requestedSize, (byte) 0); - unsafe.freeMemory(address); - } + @Override + protected long memoryAddress() { + return address; + } - @Override - public long getAllocatedSize() { - return requestedSize; - } + @Override + protected void release0() { + unsafe.setMemory(address, requestedSize, (byte) 0); + unsafe.freeMemory(address); + } + + @Override + public long getSize() { + return requestedSize; + } - private Unsafe getUnsafe() { - Field f = null; - try { - f = Unsafe.class.getDeclaredField("theUnsafe"); - f.setAccessible(true); - return (Unsafe) f.get(null); - } catch (NoSuchFieldException | IllegalAccessException e) { - throw new RuntimeException(e); - } finally { - if (f != null) { - f.setAccessible(false); - } - } + private Unsafe getUnsafe() { + Field f = null; + try { + f = Unsafe.class.getDeclaredField("theUnsafe"); + f.setAccessible(true); + return (Unsafe) f.get(null); + } catch (NoSuchFieldException | IllegalAccessException e) { + throw new RuntimeException(e); + } finally { + if (f != null) { + f.setAccessible(false); } - }).build()); + } + } + }).build()); } // Allocation listener From 10547bf698cb4ba125189bf36b95f6fbe5e03269 Mon Sep 17 00:00:00 2001 From: Rong Rong Date: Wed, 15 Jan 2020 17:58:06 -0800 Subject: [PATCH 4/5] address patch comment --- .../java/org/apache/arrow/memory/NettyAllocationManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java b/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java index 66dcd519caa..9eb5a60b344 100644 --- a/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java +++ b/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java @@ -64,7 +64,7 @@ protected void release0() { /** * Returns the underlying memory chunk size managed. * - *

NettyAllocationManager manages memory chunk by the exact requested size. + *

NettyAllocationManager rounds requested size up to the next power of two. */ @Override protected long getSize() { From 15878d102ff76fa42a839efe272fee92f6514211 Mon Sep 17 00:00:00 2001 From: Rong Rong Date: Wed, 15 Jan 2020 18:27:07 -0800 Subject: [PATCH 5/5] revert change and make API still public --- .../main/java/org/apache/arrow/memory/AllocationManager.java | 2 +- .../java/org/apache/arrow/memory/NettyAllocationManager.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java b/java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java index a6ccde55c91..50c0908b4f0 100644 --- a/java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java +++ b/java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java @@ -183,7 +183,7 @@ void release(final BufferLedger ledger) { * * @return size of underlying memory chunk */ - protected abstract long getSize(); + public abstract long getSize(); /** * Return the absolute memory address pointing to the fist byte of underling memory chunk. diff --git a/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java b/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java index 9eb5a60b344..35b02f54ecd 100644 --- a/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java +++ b/java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java @@ -67,7 +67,7 @@ protected void release0() { *

NettyAllocationManager rounds requested size up to the next power of two. */ @Override - protected long getSize() { + public long getSize() { return allocatedSize; }