ARROW-7329: [Java] AllocationManager: Allow managing different types … #5973

zhztheplayer · 2019-12-05T09:57:04Z

…of memory other than those are allocated using Netty

See https://jira.apache.org/jira/browse/ARROW-7329

…of memory other than those are allocated using Netty

github-actions · 2019-12-05T10:00:52Z

https://issues.apache.org/jira/browse/ARROW-7329

jacques-n · 2019-12-05T14:02:55Z

Nice work. Can you make a change so that allocationmanager is the base class and NettyAllocationManager is the subclass? Will make this much easier to review and track history.

zhztheplayer · 2019-12-05T14:37:54Z

@jacques-n - Sure, will do. I was to avoid breaking change to AllocationManager, but not sure if it's necessary. Thanks for your suggestion.

java/memory/src/main/java/org/apache/arrow/memory/BufferLedger.java

jacques-n · 2019-12-06T07:21:45Z

java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java

        // 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());


why the change?

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

arrow/java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java

Lines 73 to 84 in 21a4ffe

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.

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

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

jacques-n · 2019-12-06T07:22:06Z

java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java

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


I was thinking to force the implementations to be packaged under org.apache.arrow.memory... Which doesn't seem to be necessary.

Done. Also changed memoryAddress() and constructor.

jacques-n · 2019-12-06T07:23:06Z

java/memory/src/main/java/org/apache/arrow/memory/BaseAllocator.java

+  }
+
+  /**
+   * All {@link AllocationManager} instances created are defaulted to Netty implementation.


comment arguemnts

jacques-n · 2019-12-06T07:30:08Z

java/memory/src/main/java/org/apache/arrow/memory/BufferLedger.java

   * chunk. In that case, the ledger should inform the allocation manager
   * about releasing its ownership for the chunk. Whether or not the memory
-   * chunk will be released is something that {@link AllocationManager} will
+   * chunk will be released is something that {@link NettyAllocationManager} will


why change?

Seems to be mistake. Thanks for catch.

jacques-n · 2019-12-06T07:31:14Z

java/memory/src/main/java/org/apache/arrow/memory/ChildAllocator.java

-
+  @Override
+  protected AllocationManager newAllocationManager(BaseAllocator accountingAllocator, int size) {
+    // to expect consistent behavior with parent on creating allocation managers


This is kind of wonky. Let's have a factory instead.

java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java

java/memory/src/main/java/org/apache/arrow/memory/BufferLedger.java

jacques-n · 2019-12-11T08:21:27Z

java/memory/src/main/java/org/apache/arrow/memory/BaseAllocator.java

+  /**
+   * Returns a builder class for configuring BaseAllocator's options.
+   */
+  public static ConfigBuilder configBuilder() {


Can you add Immutables to the project and use that instead. Should allow us to avoid creation/management of builders. I've started using it in another project and found it quite helpful at avoiding lots of scaffolding code.

Let's add a new method which is
T unwrap(Class clazz)

Done.

Can you add Immutables to the project and use that instead.

Thanks for suggesting Immutables, I have tried and it works pretty well. The only thing I am worried about is it slightly increases the development complexity.

Done.

jacques-n · 2019-12-11T08:23:09Z

java/memory/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java

  @Test
  public void testAllocator_childRemovedOnClose() throws Exception {
-    try (final RootAllocator rootAllocator = new RootAllocator(MAX_ALLOCATION)) {
+    try (final RootAllocator rootAllocator = createRootAllocator(MAX_ALLOCATION)) {


let's avoid this change

Agreed. Something is over-tested.

This reverts commit 8a54af4.

This reverts commit 1e7f21b.

jacques-n

Sorry I missed coming back to this. This LGTM. Thanks for pulling it to completion.

+1

lidavidm

Just some quick general questions - I'm willing to go ahead and merge this though if nobody else gets to it.

java/memory/src/main/java/org/apache/arrow/memory/BaseAllocator.java

lidavidm · 2020-01-06T14:50:35Z

java/memory/pom.xml

      <groupId>org.slf4j</groupId>
      <artifactId>slf4j-api</artifactId>
    </dependency>
+    <dependency>


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.

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.

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

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

liyafan82 · 2020-01-07T02:09:29Z

java/memory/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java

-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+
+import static org.junit.Assert.*;


Please expand this to avoid style errors.

OK but It's weird that I didn't see any style errors on this.

liyafan82 · 2020-01-07T12:40:31Z

java/memory/src/main/java/org/apache/arrow/memory/NettyAllocationManager.java

+
+  private static final PooledByteBufAllocatorL INNER_ALLOCATOR = new PooledByteBufAllocatorL();
+  static final UnsafeDirectLittleEndian EMPTY = INNER_ALLOCATOR.empty;
+  static final long CHUNK_SIZE = INNER_ALLOCATOR.getChunkSize();


It seems this is not used.

Actually it is used in DefaultRoundingPolicy via reflection:

arrow/java/memory/src/main/java/org/apache/arrow/memory/rounding/DefaultRoundingPolicy.java

Lines 39 to 47 in 7bd4e73

private DefaultRoundingPolicy() {

try {

Field field = AllocationManager.class.getDeclaredField("CHUNK_SIZE");

field.setAccessible(true);

chunkSize = (Long) field.get(null);

} catch (Exception e) {

throw new RuntimeException("Failed to get chunk size from allocation manager");

}

}

jacques-n

I think this looks good. The size thing could be dealt with in a follow-on.

+1

jacques-n · 2020-01-09T18:11:35Z

java/memory/pom.xml

      <groupId>org.slf4j</groupId>
      <artifactId>slf4j-api</artifactId>
    </dependency>
+    <dependency>


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

jacques-n · 2020-01-09T18:13:22Z

java/memory/src/main/java/org/apache/arrow/memory/AllocationManager.java

        // 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());


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

jacques-n · 2020-01-09T18:15:31Z

java/memory/src/main/java/org/apache/arrow/memory/BaseAllocator.java


  @Override
-  public BufferAllocator getParentAllocator() {
+  public BaseAllocator getParentAllocator() {


It is okay to expose here as long as the BufferAllocator interface is BufferAllocator.

lidavidm · 2020-01-10T02:09:52Z

I'll go ahead and merge, then file an issue to clarify the naming. Thanks @zhztheplayer for carrying this through!

lidavidm · 2020-01-10T02:15:33Z

I filed https://issues.apache.org/jira/browse/ARROW-7538 for the actual vs desired size clarification.

zhztheplayer · 2020-01-10T03:12:33Z

Thanks for everyone's review!

This is a follow up on #5973. Closes #6204 from walterddr/ARROW-7538 and squashes the following commits: 15878d1 <Rong Rong> revert change and make API still public 10547bf <Rong Rong> address patch comment 97e7d3b <Rong Rong> address patch comments 489c203 <Rong Rong> fix checkstyle 1784f92 <Rong Rong> fix requested and allocated size difference Authored-by: Rong Rong <rongr@apache.org> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>

…of memory other than those are allocated using Netty See https://jira.apache.org/jira/browse/ARROW-7329 Closes apache#5973 from zhztheplayer/ARROW-7329 and squashes the following commits: c0345c0 <Hongze Zhang> Expand star import 81b1076 <Hongze Zhang> Introduce Immutables for config builder 214230e <Hongze Zhang> Add method BufferLedger#unwrap a38853f <Hongze Zhang> Revert "Use existing test cases in TestBaseAllocator" 1309a08 <Hongze Zhang> Revert "Visibility change" 1e7f21b <Hongze Zhang> Use existing test cases in TestBaseAllocator 8a54af4 <Hongze Zhang> Visibility change 96e3e52 <Hongze Zhang> Oops 101d529 <Hongze Zhang> Pass config to root allocator 98901aa <Hongze Zhang> Use AllocationManager.Factory c049ab6 <Hongze Zhang> getSize() was public, should keep the visibility 13629f4 <Hongze Zhang> Patch a4f96f3 <Hongze Zhang> Visibility f9891d5 <Hongze Zhang> Typo a366e99 <Hongze Zhang> Address review comments 4ba7796 <Hongze Zhang> Inconsistency on creating AllocationManager between child and parent 7627be8 <Hongze Zhang> A default implementation for AllocationManager::size() 7db394b <Hongze Zhang> Import order 8a3f79d <Hongze Zhang> Correct class names in comments 1e6e844 <Hongze Zhang> Change class names 538a543 <Hongze Zhang> ARROW-7329: AllocationManager: Allow managing different types of memory other than those are allocated using Netty Authored-by: Hongze Zhang <hongze.zhang@intel.com> Signed-off-by: David Li <li.davidm96@gmail.com>

This is a follow up on apache#5973. Closes apache#6204 from walterddr/ARROW-7538 and squashes the following commits: 15878d1 <Rong Rong> revert change and make API still public 10547bf <Rong Rong> address patch comment 97e7d3b <Rong Rong> address patch comments 489c203 <Rong Rong> fix checkstyle 1784f92 <Rong Rong> fix requested and allocated size difference Authored-by: Rong Rong <rongr@apache.org> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>

ARROW-7329: [Java] AllocationManager: Allow managing different types …

538a543

…of memory other than those are allocated using Netty

Change class names

1e6e844

zhztheplayer force-pushed the ARROW-7329 branch from fbeae99 to 1e6e844 Compare December 6, 2019 00:45

zhztheplayer added 4 commits December 6, 2019 08:48

Correct class names in comments

8a3f79d

Import order

7db394b

A default implementation for AllocationManager::size()

7627be8

Inconsistency on creating AllocationManager between child and parent

4ba7796

zhztheplayer force-pushed the ARROW-7329 branch from 4890805 to 4ba7796 Compare December 6, 2019 01:13

zhztheplayer commented Dec 6, 2019

View reviewed changes

java/memory/src/main/java/org/apache/arrow/memory/BufferLedger.java Show resolved Hide resolved

zhztheplayer marked this pull request as ready for review December 6, 2019 02:32

jacques-n reviewed Dec 6, 2019

View reviewed changes

zhztheplayer added 5 commits December 6, 2019 16:58

Address review comments

a366e99

Typo

f9891d5

Visibility

a4f96f3

Patch

13629f4

getSize() was public, should keep the visibility

c049ab6

zhztheplayer force-pushed the ARROW-7329 branch from 25d8a27 to 017f31a Compare December 6, 2019 15:56

Use AllocationManager.Factory

98901aa

zhztheplayer force-pushed the ARROW-7329 branch from 017f31a to 98901aa Compare December 6, 2019 15:59

zhztheplayer added 4 commits December 7, 2019 00:12

Pass config to root allocator

101d529

Oops

96e3e52

Visibility change

8a54af4

Use existing test cases in TestBaseAllocator

1e7f21b

zhztheplayer force-pushed the ARROW-7329 branch from 1e7a187 to 1e7f21b Compare December 11, 2019 06:49

jacques-n reviewed Dec 11, 2019

View reviewed changes

Revert "Visibility change"

1309a08

This reverts commit 8a54af4.

zhztheplayer added 2 commits December 11, 2019 20:57

Revert "Use existing test cases in TestBaseAllocator"

a38853f

This reverts commit 1e7f21b.

Add method BufferLedger#unwrap

214230e

zhztheplayer force-pushed the ARROW-7329 branch from 7fbd438 to 9e8b348 Compare December 11, 2019 15:24

Introduce Immutables for config builder

81b1076

zhztheplayer force-pushed the ARROW-7329 branch from 9e8b348 to 81b1076 Compare December 11, 2019 15:57

emkornfield mentioned this pull request Dec 12, 2019

ARROW-6720: [C++] Add Parquet Reader/Writer Adapter for JNI Bridge #5719

Closed

jacques-n approved these changes Jan 3, 2020

View reviewed changes

lidavidm reviewed Jan 6, 2020

View reviewed changes

liyafan82 reviewed Jan 7, 2020

View reviewed changes

Expand star import

c0345c0

liyafan82 approved these changes Jan 9, 2020

View reviewed changes

jacques-n reviewed Jan 9, 2020

View reviewed changes

lidavidm closed this in dff612e Jan 10, 2020

walterddr mentioned this pull request Jan 15, 2020

ARROW-7538: [Java] Clarify actual and desired size in AllocationManager #6204

Closed

This was referenced Jan 10, 2020

[Java] AllocationManager: Allow managing different types of memory other than those are allocated using Netty #16987

Closed

Clarify actual and desired size in AllocationManager #23800

Closed

	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();
	}

	private DefaultRoundingPolicy() {
	try {
	Field field = AllocationManager.class.getDeclaredField("CHUNK_SIZE");
	field.setAccessible(true);
	chunkSize = (Long) field.get(null);
	} catch (Exception e) {
	throw new RuntimeException("Failed to get chunk size from allocation manager");
	}
	}

ARROW-7329: [Java] AllocationManager: Allow managing different types … #5973

ARROW-7329: [Java] AllocationManager: Allow managing different types … #5973

Uh oh!

Conversation

zhztheplayer commented Dec 5, 2019

Uh oh!

github-actions bot commented Dec 5, 2019

Uh oh!

jacques-n commented Dec 5, 2019

Uh oh!

zhztheplayer commented Dec 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhztheplayer Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacques-n left a comment

Choose a reason for hiding this comment

Uh oh!

lidavidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacques-n left a comment

Choose a reason for hiding this comment

Uh oh!

zhztheplayer commented Dec 5, 2019 •

edited

Loading

zhztheplayer Dec 11, 2019 •

edited

Loading