Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions java/memory/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
<dependency>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth taking a dependency for a single builder? At the very least, let's file a follow-up to see if we can replace any other builders in the project with Immutables as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second the idea to factor other builders. So far I didn't have a check but we may make relevant changes in other issue/pr.

And I don't think the new dependency matters much as it's a single jar library and it is not needed in runtime.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be useful elsewhere. For example, we should really use it for the *.pojo classes. Also, as mentioned, this isn't used/required at runtime

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah - my mistake, non-runtime dependencies are even better.

<groupId>org.immutables</groupId>
<artifactId>value</artifactId>
</dependency>
</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,13 @@

import org.apache.arrow.util.Preconditions;

import io.netty.buffer.PooledByteBufAllocatorL;
import io.netty.buffer.UnsafeDirectLittleEndian;

/**
* Manages the relationship between one or more allocators and a particular UDLE. Ensures that
* The abstract base class of AllocationManager.
*
* <p>Manages the relationship between one or more allocators and a particular UDLE. Ensures that
* one allocator owns the
* memory that multiple allocators may be referencing. Manages a BufferLedger between each of its
* associated allocators.
* This class is also responsible for managing when memory is allocated and returned to the
* Netty-based
* PooledByteBufAllocatorL.
*
* <p>The only reason that this isn't package private is we're forced to put ArrowBuf in Netty's
* package which need access
Expand All @@ -47,40 +43,34 @@
* typical query. The
* contention of acquiring a lock on AllocationManager should be very low.
*/
public class AllocationManager {
public abstract class AllocationManager {

private static final AtomicLong MANAGER_ID_GENERATOR = new AtomicLong(0);
private static final PooledByteBufAllocatorL INNER_ALLOCATOR = new PooledByteBufAllocatorL();

static final UnsafeDirectLittleEndian EMPTY = INNER_ALLOCATOR.empty;
static final long CHUNK_SIZE = INNER_ALLOCATOR.getChunkSize();

private final RootAllocator root;
private final long allocatorManagerId = MANAGER_ID_GENERATOR.incrementAndGet();
private final int size;
private final UnsafeDirectLittleEndian memoryChunk;
// ARROW-1627 Trying to minimize memory overhead caused by previously used IdentityHashMap
// see JIRA for details
private final LowCostIdentityHashMap<BaseAllocator, BufferLedger> map = new LowCostIdentityHashMap<>();
private final long amCreationTime = System.nanoTime();
private final int size;

// The ReferenceManager created at the time of creation of this AllocationManager
// is treated as the owning reference manager for the underlying chunk of memory
// managed by this allocation manager
private volatile BufferLedger owningLedger;
private volatile long amDestructionTime = 0;

AllocationManager(BaseAllocator accountingAllocator, int size) {
protected AllocationManager(BaseAllocator accountingAllocator, int size) {
Preconditions.checkNotNull(accountingAllocator);
accountingAllocator.assertOpen();

this.size = size;
this.root = accountingAllocator.root;
this.memoryChunk = INNER_ALLOCATOR.allocate(size);

// we do a no retain association since our creator will want to retrieve the newly created
// ledger and will create a reference count at that point
this.owningLedger = associate(accountingAllocator, false);
this.size = memoryChunk.capacity();
}

BufferLedger getOwningLedger() {
Expand All @@ -91,14 +81,6 @@ void setOwningLedger(final BufferLedger ledger) {
this.owningLedger = ledger;
}

/**
* Get the underlying memory chunk managed by this AllocationManager.
* @return buffer
*/
UnsafeDirectLittleEndian getMemoryChunk() {
return memoryChunk;
}

/**
* Associate the existing underlying buffer with a new allocator. This will increase the
* reference count on the corresponding buffer ledger by 1
Expand Down Expand Up @@ -172,10 +154,10 @@ void release(final BufferLedger ledger) {
// the only <allocator, reference manager> 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(size);
((BaseAllocator)oldLedger.getAllocator()).releaseBytes(getSize());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to keep up with the current behaviour. Following is the constructor code of current AllocationManager:

AllocationManager(BaseAllocator accountingAllocator, int size) {
Preconditions.checkNotNull(accountingAllocator);
accountingAllocator.assertOpen();
this.root = accountingAllocator.root;
this.memoryChunk = INNER_ALLOCATOR.allocate(size);
// we do a no retain association since our creator will want to retrieve the newly created
// ledger and will create a reference count at that point
this.owningLedger = associate(accountingAllocator, false);
this.size = memoryChunk.capacity();
}

This allows an AllocationManager allocates different bytes of memory comparing to desired. So at here, size is desired size passed by constructor, while getSize() should be the actual size. I'll do some name changes in case of confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If size and getSize() are different, maybe this should be made explicit in the JavaDoc?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, maybe call one requested_size and one size (or similar)

// free the memory chunk associated with the allocation manager
memoryChunk.release();
((BaseAllocator)oldLedger.getAllocator()).getListener().onRelease(size);
release0();
((BaseAllocator)oldLedger.getAllocator()).getListener().onRelease(getSize());
amDestructionTime = System.nanoTime();
owningLedger = null;
} else {
Expand All @@ -198,9 +180,37 @@ void release(final BufferLedger ledger) {

/**
* Return the size of underlying chunk of memory managed by this Allocation Manager.
* @return size of memory chunk
*
* @return size of underlying memory chunk
*/
public int getSize() {
return size;
}

/**
* Return the absolute memory address pointing to the fist byte of underling memory chunk.
*/
protected abstract long memoryAddress();

/**
* Release the underling memory chunk.
*/
protected abstract void release0();

/**
* A factory interface for creating {@link AllocationManager}.
* One may extend this interface to use a user-defined AllocationManager implementation.
*/
public interface Factory {

/**
* Create an {@link AllocationManager}.
*
* @param accountingAllocator The allocator that are expected to be associated with newly created AllocationManager.
* Currently it is always equivalent to "this"
* @param size Size (in bytes) of memory managed by the AllocationManager
* @return The created AllocationManager used by this allocator
*/
AllocationManager create(BaseAllocator accountingAllocator, int size);
}
}
Loading