ARROW-16673: [Java] Integrate C Data into allocator hierarchy #14506

lidavidm · 2022-10-25T20:39:02Z

Document some of the memory management internals
Note that in practice, you have to use the concrete implementations of certain interfaces
Begin integrating C Data into the full memory management stack instead of trying to patch it in at the edges

java/c/src/main/java/org/apache/arrow/c/ArrayImporter.java

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

github-actions · 2022-10-25T21:37:34Z

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

lidavidm · 2022-10-26T13:10:14Z

TODOs

Refactor type handling to be entirely inside the visitor, so we can handle varlen types
Reconcile docs with the markdown document describing the design
Add this case to the roundtrip tests (and possibly refactor those tests to be more extensible?)
Make sure new APIs are documented
Remove any local testing code

lwhite1

Looks good to me so far.

java/memory/memory-core/src/main/java/org/apache/arrow/memory/ForeignAllocation.java

lwhite1 · 2022-10-26T12:03:24Z

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

@@ -18,8 +18,10 @@
 package org.apache.arrow.memory;

 /**
- * Reference Manager manages one or more ArrowBufs that share the
- * reference count for the underlying memory chunk.
+ * ReferenceManager is the reference count for one or more allocations.


I'm not sure "ReferenceManager is the reference count..." is completely accurate. Maybe "ReferenceManager is the reference count manager..." or "ReferenceManager tracks the reference count..."

What is the difference between these? Given the public interface is a set of reference counting methods, I'd say it is a reference count. (And for instance, the way you manipulate the reference count of an ArrowBuf is by getting its reference manager.)

it's a nit for sure. Ignore it

lwhite1 · 2022-10-26T12:11:15Z

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

@@ -91,7 +95,7 @@ public interface ReferenceManager {
  /**
   * Transfer the memory accounting ownership of this ArrowBuf to another allocator.
   * This will generate a new ArrowBuf that carries an association with the underlying memory
-   * for the given ArrowBuf


I think this method comment is misleading. While it can transfer the memory accounting ownership ... to another allocator, it can also create a new ArrowBuf in the same allocator if the allocator argument is the same as the existing allocator. This can be useful for creating a new Vector over the same underlying memory.

I did try to reword this a bit, hope that looks better.

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

java/c/src/main/java/org/apache/arrow/c/ArrayImporter.java

lidavidm · 2022-10-27T00:25:45Z

Still haven't updated docs, but all types are implemented now. There's two failure modes that need tests, then fixes.

lwhite1 · 2022-10-27T13:25:12Z

java/c/src/main/java/org/apache/arrow/c/CDataArrayHandle.java

+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.ForeignAllocationManager;
+
+/** Manage the reference count of an imported C Data ArrowArray. */


Looks good, but I'm a bit concerned that the class name and the fact that it does not implement ReferenceManager obscures the fact that it does reference counting.

There is a fundamental mismatch in memory handling designs and this cannot implement ReferenceManager. I will update the docstring and reconsider the name but I do not think anything else is possible, not without entirely overhauling the core implementation. Even if in theory we can work with only the interfaces provided, in practice we need to fit into the concrete implementations, and so BufferLedger is unavoidable.

that's fine.

I did look at it a little bit. But I don't think you can use any implementation of ReferenceManager but BufferLedger if you want everything else to work. It does feel bad to have effectively two redundant reference counts.

I understand. It was a 'it would be nice if....' comment

lwhite1 · 2022-10-28T14:50:19Z

@lidavidm I would like to update the tutorial docs for memory management after this is implemented to make sure I fully understand it all, assuming that works for you.

lidavidm · 2022-10-28T15:08:02Z

That would be much appreciated, thank you. It would be good if we could all check our understanding.

lidavidm · 2022-10-28T15:11:27Z

FWIW, I think this is ready. Maybe @emkornfield and/or @zhztheplayer would also like to take a look and see how the proposed solution works?

lwhite1 · 2022-11-08T14:57:11Z

@emkornfield and/or @zhztheplayer, do you think you will have time to consider this? Thanks

lwhite1

Looks good to me.

zhztheplayer

Thanks!

BTW In future maybe we can consider integrating AllocationListener into this allocation path too?

zhztheplayer · 2022-11-10T05:38:56Z

java/memory/memory-netty/src/test/java/org/apache/arrow/memory/TestForeignAllocation.java

+    UnsafeForeignAllocationManager allocationManager = new UnsafeForeignAllocationManager(allocator, bufferSize);
+    try {
+      assertEquals(0, allocator.getAllocatedMemory());
+      ArrowBuf buf = allocator.wrapForeignAllocation(allocationManager);


Although the code already works greatly I think we may have chance to optimize such usages by decoupling BufferAllocator from AllocationManager? @lidavidm @lwhite1

What sort of optimization are you thinking?

It would be nice to avoid the double reference count but I think that will require some more intricate surgery on the allocator internals.

What sort of optimization are you thinking?

To make AllocationManager creatable without an allocator, e.g.

UnsafeForeignAllocationManager allocationManager = new UnsafeForeignAllocationManager(bufferSize); ArrowBuf buf = allocator.wrapForeignAllocation(allocationManager);

It would be nice to avoid the double reference count but I think that will require some more intricate surgery on the allocator internals.

(Sorry for asking more on an already approved PR, indeed these are all non-blocker questions)

Regarding the double reference counting, I am not sure about why it's actually needed? In the previous PR we didn't actually touch anything about ref counting and things went well.

It didn't actually implement the API. It just suppressed an exception.

new AllocationManager(allocator) creates an zero ref count owning ledger. I guess that ledger may lead to unexpected behavior when user call wrapForeignAllocation(...) from another allocator. Maybe decoupling allocator from AllocationManager's constructor (not the manager itself) will be a solution.

Again, I don't see what the issue is. That ledger is the point, it's what links the allocation to the allocator.

You shouldn't be wrapping the same allocation twice. Not sure we can really do anything about that, this is inherently a somewhat unsafe API.

I considered something like:

BufferAllocator child1 = ...; BufferAllocator child2 = ...; ForeignAllocationManager allocationManager = new ForeignAllocationManager(child1, size, memoryAddress); ArrowBuf buf = child2.wrapForeignAllocation(allocationManager);

It looks like optimizable if we tell user it's a must to instantly call wrapForeignAllocation on same allocator after creating the AM.

Ah, ok, thanks. I think I see now. I'll give that a test, and I think I might change it so that the ForeignAllocation is only a release callback, pointer, and size. That way the details can all be properly encapsulated.

See the latest commit.

zhztheplayer · 2022-11-10T17:27:01Z

java/c/src/main/java/org/apache/arrow/c/ReferenceCountedArrowArray.java

+ * <p>There is a fundamental mismatch here between memory allocation schemes: AllocationManager represents a single
+ * allocation (= a single address and length). But an ArrowArray combines multiple allocations behind a single
+ * deallocation callback. This class bridges the two by tracking a reference count, so that the single callback


But an ArrowArray combines multiple allocations behind a single deallocation callback.

Wasn't the deallocation of an ArrowArray always follow the postorder-traversal of the array tree? Or maybe I just got something wrong about the deallocation process.

The order does not matter here. It is about one vs many.

Yes I got it after going through the code. Thought the comment is related to the struct array -> child array case.

Ah ok yeah. I removed the original comment about the struct/child array case since it is just not possible to separate the child allocations from the parent, unfortunately.

zhztheplayer · 2022-11-22T07:34:02Z

java/memory/memory-core/src/main/java/org/apache/arrow/memory/ForeignAllocation.java

+/**
+ * An allocation of memory that does not come from a BufferAllocator, but rather an outside source (like JNI).
+ */
+public abstract class ForeignAllocation {


lwhite1 · 2022-11-22T14:25:33Z

@lidavidm Is it possible to merge this PR today?

lidavidm · 2022-11-22T14:50:46Z

Given that it's touching some core APIs, I'd like to see some review…I can go and make it clear that the APIs added here are experimental, at least for now

lwhite1 · 2022-11-22T15:12:26Z

Given that it's touching some core APIs, I'd like to see some review…I can go and make it clear that the APIs added here are experimental, at least for now

IDK if you saw @zhztheplayer's approval from last night. I've been following along, but LMK if you want me to formally approve the latest changes.

lidavidm · 2022-11-22T19:19:51Z

Yes, and it's appreciated; I was hoping @emkornfield might also be able to comment on the proposed design, though. It shouldn't hold us back, I just want to go through and make it clear where the new APIs are experimental or meant for internal use.

lidavidm · 2022-11-23T16:26:26Z

I'll merge this for now and we can revisit the design if issues come up.

ursabot · 2022-11-25T12:12:48Z

Benchmark runs are scheduled for baseline = b1f65ea and contender = 7a47e8d. 7a47e8d is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.23% ⬆️0.03%] test-mac-arm
[Finished ⬇️0.54% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.17% ⬆️0.14%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 7a47e8dc ec2-t3-xlarge-us-east-2
[Finished] 7a47e8dc test-mac-arm
[Finished] 7a47e8dc ursa-i9-9960x
[Finished] 7a47e8dc ursa-thinkcentre-m75q
[Finished] b1f65ea4 ec2-t3-xlarge-us-east-2
[Finished] b1f65ea4 test-mac-arm
[Finished] b1f65ea4 ursa-i9-9960x
[Finished] b1f65ea4 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

This PR bumps Apache Arrow version from 10.0.0 to 11.0.0. Main changes related to PyAmber: ## Java/Scala side: - Distribute Apple M1 compatible JNI libraries via mavencentral ([#14472](apache/arrow#14472)). - Improve performance by short-circuiting null checks when comparing non null field types ([#15106](apache/arrow#15106)). - Extend Table copy functionality, and support returning copies of individual vectors ([#14389](apache/arrow#14389)). - Several enhancements to dictionary encoding ([#14891](apache/arrow#14891), ([#14902](apache/arrow#14902), ([#14874](apache/arrow#14874)). - Extend Table to support additional vector types ([#14573](apache/arrow#14573)). - Enhance and simplify handling of allocation management by integrating C Data into allocator hierarchy ([#14506](apache/arrow#14506)). ## Python side: - PyArrow now requires pandas >= 1.0 ([ARROW-18173](https://issues.apache.org/jira/browse/ARROW-18173)). - Added support for the [DataFrame Interchange Protocol](https://data-apis.org/dataframe-protocol/latest/purpose_and_scope.html) for pyarrow.Table ([GH-33346](apache/arrow#33346)). - Support for custom metadata of record batches in the IPC read and write APIs ([ARROW-16430](https://issues.apache.org/jira/browse/ARROW-16430)). - The Time32Scalar, Time64Scalar, Date32Scalar and Date64Scalar classes got a .value attribute to access the underlying integer value, similar to the other date-time related scalars ([ARROW-18264](https://issues.apache.org/jira/browse/ARROW-18264)). - Casting to string is now supported for duration ([ARROW-15822](https://issues.apache.org/jira/browse/ARROW-15822)) and decimal ([ARROW-17458](https://issues.apache.org/jira/browse/ARROW-17458)) types, which also means those can now be written to CSV. ## Issues fixed: - Now Do_action (from Python server back to Java Client) is returning a stream of results properly, and it alerts when the results are not fully consumed by the client. Such results will be used to send the flow control credits back from the Python side. We limit the results to be exact 1 for now, although it can be a stream. - Fix a bug in the Python proxy server, when unregistered action is invoked, it should not parse and return the results.

lidavidm mentioned this pull request Oct 25, 2022

ARROW-16673: [Java] C data interface: Allow ownership transferring for imported buffer #13248

Closed

lidavidm commented Oct 25, 2022

View reviewed changes

java/c/src/main/java/org/apache/arrow/c/ArrayImporter.java Show resolved Hide resolved

lidavidm commented Oct 25, 2022

View reviewed changes

java/c/src/main/java/org/apache/arrow/c/ArrayImporter.java Outdated Show resolved Hide resolved

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

lidavidm force-pushed the arrow-16673 branch from 6eca800 to fad3f49 Compare October 25, 2022 23:04

github-actions bot added the Component: Java label Oct 25, 2022

lwhite1 reviewed Oct 26, 2022

View reviewed changes

lidavidm force-pushed the arrow-16673 branch 2 times, most recently from 497bb3c to edaf4ee Compare October 27, 2022 00:25

lidavidm force-pushed the arrow-16673 branch from edaf4ee to b1164b0 Compare October 27, 2022 00:32

lidavidm changed the title ~~ARROW-16673: [Java] WIP: proper handling of imported buffers~~ ARROW-16673: [Java] Integrate C Data into allocator hierarchy Oct 27, 2022

lwhite1 reviewed Oct 27, 2022

View reviewed changes

lidavidm force-pushed the arrow-16673 branch from b1164b0 to 8af641f Compare October 27, 2022 16:04

lidavidm marked this pull request as ready for review October 27, 2022 16:04

lwhite1 approved these changes Nov 8, 2022

View reviewed changes

zhztheplayer approved these changes Nov 10, 2022

View reviewed changes

zhztheplayer reviewed Nov 10, 2022

View reviewed changes

lidavidm force-pushed the arrow-16673 branch 2 times, most recently from 39a7f29 to aa8c22e Compare November 15, 2022 21:10

zhztheplayer approved these changes Nov 22, 2022

View reviewed changes

zhztheplayer reviewed Nov 22, 2022

View reviewed changes

lidavidm added 3 commits November 23, 2022 08:19

ARROW-16673: [Java] Integrate C Data into allocator hierarchy

457ebe6

ARROW-16673: [Java] Encapsulate more details of memory management

044415c

ARROW-16673: [Java] Mark new APIs experimental

92a7fc8

lidavidm force-pushed the arrow-16673 branch from aa8c22e to 92a7fc8 Compare November 23, 2022 13:22

lidavidm merged commit 7a47e8d into apache:master Nov 23, 2022

lidavidm deleted the arrow-16673 branch November 23, 2022 16:27

asfimport mentioned this pull request Nov 25, 2022

[Java] C data interface: Allow ownership transferring for imported buffer #32019

Closed

zhztheplayer mentioned this pull request Feb 9, 2023

Fix int overflow when the struct column has null value oap-project/arrow#199

Merged

Yicong-Huang mentioned this pull request May 1, 2023

Bump Apache Arrow to 11.0.0 Texera/texera#1900

Merged

ARROW-16673: [Java] Integrate C Data into allocator hierarchy #14506

ARROW-16673: [Java] Integrate C Data into allocator hierarchy #14506

Conversation

lidavidm commented Oct 25, 2022 • edited Loading

github-actions bot commented Oct 25, 2022

lidavidm commented Oct 26, 2022

lwhite1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidavidm commented Oct 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwhite1 commented Oct 28, 2022

lidavidm commented Oct 28, 2022

lidavidm commented Oct 28, 2022

lwhite1 commented Nov 8, 2022

lwhite1 left a comment

Choose a reason for hiding this comment

zhztheplayer left a comment • edited Loading

Choose a reason for hiding this comment

zhztheplayer Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhztheplayer Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwhite1 commented Nov 22, 2022

lidavidm commented Nov 22, 2022

lwhite1 commented Nov 22, 2022

lidavidm commented Nov 22, 2022

lidavidm commented Nov 23, 2022

ursabot commented Nov 25, 2022

lidavidm commented Oct 25, 2022 •

edited

Loading

zhztheplayer left a comment •

edited

Loading

zhztheplayer Nov 10, 2022 •

edited

Loading

zhztheplayer Nov 10, 2022 •

edited

Loading