ARROW-3191: [Java] Make ArrowBuf work with arbitrary underlying memory #4151

siddharthteotia · 2019-04-13T18:22:02Z

This patch has the following goals:

(1) Make ArrowBuf work with any arbitrary memory.
(2) Decouple the usage of data get/set in ArrowBuf and memory accounting, reference management, ownership etc.

Changes

(1) A ReferenceManager interface that can be provided to ArrowBuf. This allows the users to provide their own custom implementation of reference management or it can be a NO-OP.
(2) All the accounting, ownership, reference related APIs have been moved to the default implementation of ReferenceManager -- BufferLedger, AllocationManager
(3) ArrowBuf is now literally an abstraction over some user provided underlying memory chunk. All it needs is starting virtual address and length of data to access along with a user provided implementation of ReferenceManager.
(4) ArrowBuf no longer extends or implements any of Netty's buffer interfaces. Thus all of the extra and unused APIs have been removed and it just provides simple get/set.

There is quite a bit of cleanup that needs to be done since some APIs have been moved out of ArrowBuf. So the caller code needs to change. They are likely going to be boilerplate changes but I would like to do them once we have consensus on the major set of changes here and the decoupling between ArrowBuf usage and reference management.

So the code doesn't compile yet because of the above mentioned reason. Secondly, there are a few things that I have removed assuming they are not being used -- like BufferManager in ArrowBuf. I am still evaluating its usage. So there a few TODOs in code for these reasons.

Raising PR before the code is complete to get feedback on the important set of changes.

pearu · 2019-04-14T09:42:16Z

Make ArrowBuf work with any arbitrary memory.

What does "arbitrary memory" mean? Would this include also device memory?

emkornfield · 2019-04-14T15:37:34Z

java/memory/src/main/java/io/netty/buffer/ArrowBuf.java

what do you think about changing the API to take LONGs here instead? Or do you think this should be a separate PR?

I don't think we should encumber this patch with that change.

Let's also have a discussion about that change on the mailing list before we make it. What use case is there where this is important?

I can bring this up for discussion on the mailing list but the suggestion is to bring this in line with the C++ side of things so large message batches can be read in java if desired see (https://issues.apache.org/jira/browse/ARROW-679)

emkornfield · 2019-04-14T15:46:54Z

java/memory/src/main/java/io/netty/buffer/ArrowBuf.java

can we change this to either take a char, or replicate documentation saying which bits are ignored.

Actually, is there a use-case to store 2-byte characters in Arrow (I'm not thinking of one off the top of my head).

emkornfield · 2019-04-14T15:48:08Z

java/memory/src/main/java/io/netty/buffer/ArrowBuf.java

eliminate or document.

emkornfield · 2019-04-14T15:48:23Z

java/memory/src/main/java/io/netty/buffer/ArrowBuf.java

Please try to add javadoc to each method

Let's make sure people are happy with overall shape/approach of patch before we spend time on this type of changes.

emkornfield · 2019-04-14T15:49:41Z

java/memory/src/main/java/io/netty/buffer/ArrowBuf.java

consider returning a collection here (if it isn't too big a change, even even we want to change to support 64 bit size memory in a separate PR).

emkornfield · 2019-04-14T16:02:11Z

java/memory/src/main/java/io/netty/buffer/ArrowBuf.java

generally same question about changing the API to use Longs instead of Ints for all getters/settings

emkornfield · 2019-04-14T16:21:12Z

java/memory/src/main/java/io/netty/buffer/ArrowBuf.java

should this be tied to memory? What if we want a file backed implementation?

It definitely should be tied to a memory address. Whether that is backed by a memory mapped file will be up to the consumer with this change.

jacques-n · 2019-04-15T04:55:03Z

Make ArrowBuf work with any arbitrary memory.

What does "arbitrary memory" mean? Would this include also device memory?

It just means any memory address, not just those attached to a Netty allocator. Right now, ArrowBuf must be allocated by the Netty allocator which means that if a user allocated memory some other way, he is unable to connect it to Arrow. For example, the Java Plasma client has that problem right now.

emkornfield · 2019-04-15T05:30:28Z

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

this name doesn't seem to match up well with the documentation if an actual arrowbuf is being allocated.

BryanCutler

+1 on the changes, looks great @siddharthteotia ! I just had some minor comments

BryanCutler · 2019-04-17T16:15:02Z

java/memory/src/main/java/io/netty/buffer/ArrowBuf.java

do you need a ++address here?

should be fixed in the latest commit

BryanCutler · 2019-04-17T16:16:14Z

java/memory/src/main/java/io/netty/buffer/ArrowBuf.java

Do you need to decrement length and increment dstAddress?

should be fixed in latest commit

BryanCutler · 2019-04-17T16:27:01Z

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

it returns a boolean though?

yes the interface in old ArrowBuf had the semantics that it will return true if ref count has dropped to 0, false otherwise.

BryanCutler · 2019-04-17T16:27:12Z

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

BryanCutler · 2019-04-17T16:28:19Z

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

Maybe say decrement by 1?

BryanCutler · 2019-04-17T16:34:01Z

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

could it be helpful to return the new count?

I guess why do you need this since retain() is below?

These interfaces are borrowed from how they were earlier in ArrowBuf. I didn't want to change or remove this reference count related APIs

Ok, then I agree not to change it. I thought it was a new api.

BryanCutler · 2019-04-17T16:42:33Z

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

Again here, could it be useful to return the new reference count?

May be. I just kept the way it was in previous implementation of ArrowBuf.

BryanCutler · 2019-04-17T16:45:49Z

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

I guess if it's overlimit then it reallocates? Maybe you could clarify

siddharthteotia · 2019-04-18T22:36:52Z

Thanks @jacques-n , @BryanCutler, @emkornfield , @pearu for the comments so far. I will be addressing them soon. In my latest commit, I have made all the necessary changes in java code to work with new ArrowBuf and ReferenceManager interface. More importantly, there is a wrapper buffer NettyArrowBuf interface to comply with usage in RPC and Netty related code. It will be good to get feedback on this one as well. I am also adding javadocs for the existing/new interfaces in ArrowBuf but might have missed out a few methods here and there.

As of now, the java modules build fine but I have to fix test failures. That is in progress.

yuruiz · 2019-04-23T08:19:35Z

Please update the org/apache/arrow/memory/README.md to explain how does the ReferenceManager fit into Arrow Memory Management.

and fix build issues

usage in Netty framework

and gandiva build errors

codecov-io · 2019-05-04T00:34:34Z

Codecov Report

Merging #4151 into master will increase coverage by 1.67%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4151      +/-   ##
==========================================
+ Coverage   87.77%   89.45%   +1.67%     
==========================================
  Files         758      620     -138     
  Lines       92506    83442    -9064     
  Branches     1251        0    -1251     
==========================================
- Hits        81201    74644    -6557     
+ Misses      11188     8798    -2390     
+ Partials      117        0     -117

Impacted Files	Coverage Δ
python/pyarrow/jvm.py	`95.86% <100%> (ø)`	⬆️
cpp/src/arrow/array/builder_union.h	`61.9% <0%> (-38.1%)`	⬇️
cpp/src/arrow/array/builder_base.cc	`76.36% <0%> (-5.99%)`	⬇️
cpp/src/arrow/array/builder_dict.cc	`64.41% <0%> (-3.33%)`	⬇️
cpp/src/arrow/array/builder_base.h	`94.11% <0%> (-2.44%)`	⬇️
cpp/src/parquet/statistics.cc	`87.96% <0%> (-2.3%)`	⬇️
cpp/src/arrow/compute/kernel.h	`62.76% <0%> (-2.07%)`	⬇️
cpp/src/plasma/thirdparty/ae/ae.c	`70.75% <0%> (-0.95%)`	⬇️
cpp/src/arrow/array/builder_binary.h	`97% <0%> (-0.81%)`	⬇️
python/pyarrow/tests/test_array.py	`96.31% <0%> (-0.49%)`	⬇️
... and 226 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 532450d...6bb9cfc. Read the comment docs.

siddharthteotia · 2019-05-06T18:20:35Z

I got a clean build with my previous commit. The latest commit just has come cleanup and I expect it to be clean. Please review the changes. We should plan to merge this soon.

siddharthteotia · 2019-05-07T22:19:38Z

Can this be merged?

BryanCutler

LGTM

BryanCutler · 2019-05-07T23:30:57Z

merged to master, thanks @siddharthteotia

siddharthteotia · 2019-05-08T00:38:34Z

merged to master, thanks @siddharthteotia

Thank you, @BryanCutler

jacques-n · 2019-05-08T05:16:28Z

I think @pravindra and I both wanted to do another pass over this before merging. I guess we'll open up follow-on JIRAs to address additional comments.

jacques-n

Some initial comments. Still going through it.

jacques-n · 2019-05-08T05:18:58Z

java/memory/src/main/java/io/netty/buffer/ArrowBuf.java

+   * @return netty compliant {@link NettyArrowBuf}
   */
-  public TransferResult transferOwnership(BufferAllocator target) {
+  public NettyArrowBuf asNettyBuffer() {


Netty should not be exposed in the public API. Please remove

jacques-n · 2019-05-08T05:19:35Z

java/memory/src/main/java/io/netty/buffer/ArrowBuf.java

+ * </p>
 */
-public final class ArrowBuf extends AbstractByteBuf implements AutoCloseable {
+public final class ArrowBuf implements AutoCloseable {


This should no longer be in the io.netty.buffer package.

jacques-n · 2019-05-08T05:21:19Z

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

+ * Reference Manager manages one or more ArrowBufs that share the
+ * reference count for the underlying memory chunk.
+ */
+public interface ReferenceManager {


Please provide a default implementation with noop operations

jacques-n · 2019-05-08T05:24:05Z

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

 import io.netty.buffer.ArrowBuf;
-import io.netty.buffer.ArrowBuf.TransferResult;

 public class TestBaseAllocator {


We need a set of tests that prove that this jira does what it says it does. Let's create a simple allocator that doesn't use netty and then use the vector apis doing that. This should probably be a new test class (as opposed to being added to this class).

jacques-n · 2019-05-08T05:26:45Z

java/memory/src/main/java/io/netty/buffer/ArrowBuf.java

-  private final BufferManager bufManager;
-  private final ArrowByteBufAllocator alloc;
  private final boolean isEmpty;
+  private int readerIndex;


It seems like these shoudn't be part of ArrowBuf and can be an addon/new class if people want to have index based behavior.

jacques-n · 2019-05-08T05:28:36Z

java/memory/src/main/java/io/netty/buffer/ArrowBuf.java

-  @Override
-  public ByteBuffer[] nioBuffers() {
-    return new ByteBuffer[] {nioBuffer()};
+    return isEmpty ? ByteBuffer.allocateDirect(0) : asNettyBuffer().nioBuffer(index, length);


Weird that this delegates to netty buffer since ideally ArrowBuf shouldn't be depending on Netty.

BryanCutler · 2019-05-08T17:48:22Z

Apologies @jacques-n and @pravindra if I merged too soon , I thought there were no more comments

pravindra · 2019-05-09T04:46:15Z

Apologies @jacques-n and @pravindra if I merged too soon , I thought there were no more comments

no problem at all. we'll address issues, if any, in follow-up jiras.

This patch has the following goals: (1) Make ArrowBuf work with any arbitrary memory. (2) Decouple the usage of data get/set in ArrowBuf and memory accounting, reference management, ownership etc. Changes (1) A ReferenceManager interface that can be provided to ArrowBuf. This allows the users to provide their own custom implementation of reference management or it can be a NO-OP. (2) All the accounting, ownership, reference related APIs have been moved to the default implementation of ReferenceManager -- BufferLedger, AllocationManager (3) ArrowBuf is now literally an abstraction over some user provided underlying memory chunk. All it needs is starting virtual address and length of data to access along with a user provided implementation of ReferenceManager. (4) ArrowBuf no longer extends or implements any of Netty's buffer interfaces. Thus all of the extra and unused APIs have been removed and it just provides simple get/set. There is quite a bit of cleanup that needs to be done since some APIs have been moved out of ArrowBuf. So the caller code needs to change. They are likely going to be boilerplate changes but I would like to do them once we have consensus on the major set of changes here and the decoupling between ArrowBuf usage and reference management. So the code doesn't compile yet because of the above mentioned reason. Secondly, there are a few things that I have removed assuming they are not being used -- like BufferManager in ArrowBuf. I am still evaluating its usage. So there a few TODOs in code for these reasons. Raising PR before the code is complete to get feedback on the important set of changes. Author: siddharth <siddharth@dremio.com> Closes apache#4151 from siddharthteotia/ARROW-3191 and squashes the following commits: 6bb9cfc <siddharth> Cleanup 2fa139c <siddharth> integration test issues 283916f <siddharth> Fix integration test issues 47a303b <siddharth> Setting io.netty.tryReflectionSetAccessible to true 76096e5 <siddharth> Refactor NettyArrowBuf 0b9d5d2 <siddharth> Fix test failures happening in non-debug mode and gandiva build errors 2fbb2c5 <siddharth> Fix some test failures and rebase 9e8beb6 <siddharth> Fix test failures, add javadoc and wrapper over ArrowBuf for usage in Netty framework 348be8b <siddharth> Change callers of ArrowBuf APIs to use ReferenceManager interface and fix build issues 68fe274 <siddharth> ARROW-3191: WIP for pointing ArrowBuf to arbitrary memory

emkornfield reviewed Apr 14, 2019

View reviewed changes

java/memory/src/main/java/io/netty/buffer/ArrowBuf.java Outdated

Copy link

Contributor

emkornfield Apr 14, 2019

Choose a reason for hiding this comment

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

eliminate or document.

emkornfield reviewed Apr 14, 2019

View reviewed changes

emkornfield reviewed Apr 15, 2019

View reviewed changes

BryanCutler reviewed Apr 17, 2019

View reviewed changes

siddharthteotia added 4 commits April 23, 2019 16:40

ARROW-3191:[JAVA] WIP for pointing ArrowBuf to arbitrary memory

68fe274

Change callers of ArrowBuf APIs to use ReferenceManager interface

348be8b

and fix build issues

Fix test failures, add javadoc and wrapper over ArrowBuf for

9e8beb6

usage in Netty framework

Fix some test failures and rebase

2fbb2c5

siddharthteotia force-pushed the ARROW-3191 branch from 4a4ba75 to 2fbb2c5 Compare April 23, 2019 23:47

siddharthteotia added 5 commits April 24, 2019 10:02

Fix test failures happening in non-debug mode

0b9d5d2

and gandiva build errors

Refactor NettyArrowBuf

76096e5

Setting io.netty.tryReflectionSetAccessible to true

47a303b

Fix integration test issues

283916f

integration test issues

2fa139c

kou changed the title ~~ARROW-3191:[JAVA] WIP for pointing ArrowBuf to arbitrary memory~~ ARROW-3191: [Java] WIP for pointing ArrowBuf to arbitrary memory May 4, 2019

Cleanup

6bb9cfc

siddharthteotia changed the title ~~ARROW-3191: [Java] WIP for pointing ArrowBuf to arbitrary memory~~ ARROW-3191: [Java] Make ArrowBuf work with arbitrary underlying memory May 6, 2019

BryanCutler approved these changes May 7, 2019

View reviewed changes

BryanCutler closed this in 34b9bb2 May 7, 2019

jacques-n reviewed May 8, 2019

View reviewed changes

asfimport mentioned this pull request May 29, 2019

[Java] Add support for ArrowBuf to point to arbitrary memory. #19536

Closed

ARROW-3191: [Java] Make ArrowBuf work with arbitrary underlying memory #4151

ARROW-3191: [Java] Make ArrowBuf work with arbitrary underlying memory #4151

Uh oh!

Conversation

siddharthteotia commented Apr 13, 2019

Uh oh!

pearu commented Apr 14, 2019

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!

jacques-n commented Apr 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanCutler left a comment

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!

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!

siddharthteotia commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

siddharthteotia commented Apr 18, 2019 •

edited

Loading

yuruiz commented Apr 23, 2019 •

edited

Loading

codecov-io commented May 4, 2019 •

edited

Loading