Uh oh!
There was an error while loading. Please reload this page.
Introduce a reference counted layer between ArrowBuffer and memory - #297
Conversation
…lying memory to allow buffers to be shared between multiple arrays.
There was a problem hiding this comment.
Pull request overview
Introduces reference-counted buffer ownership to allow safe sharing of underlying memory across arrays/slices and to support exporting managed buffers via the C Data interface without invalidating the original C# objects.
Changes:
- Add
SharedMemoryOwner/SharedMemoryHandleand updateArrowBufferto supportRetain()-based sharing and export. - Add shared-slice APIs (
SliceShared) acrossArrayData,Array,ChunkedArray,Column, andRecordBatch. - Update C Data export behavior/documentation and expand tests to validate reference-counted lifetimes and export semantics.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Apache.Arrow.Tests/CDataInterfacePythonTests.cs | Removes managed-export toggle usage in Python interop tests. |
| test/Apache.Arrow.Tests/CDataInterfaceDataTests.cs | Adds a regression test ensuring export doesn’t invalidate the source array. |
| test/Apache.Arrow.Tests/ArrowBufferTests.cs | Adds tests for ArrowBuffer.Retain() and SliceShared lifetime behavior. |
| test/Apache.Arrow.Tests/ArrayDataReferenceCountingTests.cs | New comprehensive tests around shared slicing/export and allocator tracking. |
| src/Apache.Arrow/RecordBatch.cs | Adds RecordBatch.SliceShared. |
| src/Apache.Arrow/Memory/SharedMemoryOwner.cs | New ref-counted wrapper around IMemoryOwner<byte>. |
| src/Apache.Arrow/Memory/SharedMemoryHandle.cs | New retainable handle implementing IMemoryOwner<byte>. |
| src/Apache.Arrow/Memory/NativeMemoryManager.cs | Removes old IOwnableAllocation path. |
| src/Apache.Arrow/Memory/ExportedAllocationOwner.cs | Updates export pin/ownership tracking to handle shared handles. |
| src/Apache.Arrow/Column.cs | Makes Column disposable and adds SliceShared. |
| src/Apache.Arrow/ChunkedArray.cs | Makes ChunkedArray disposable and adds SliceShared. |
| src/Apache.Arrow/C/CArrowArrayExporter.cs | Updates docs + deprecates managed-export toggle as ignored/obsolete. |
| src/Apache.Arrow/ArrowBuffer.cs | Replaces direct owner with shared handle and adds Retain() + new export behavior. |
| src/Apache.Arrow/Arrays/ArrowArrayFactory.cs | Adds SliceShared factory method. |
| src/Apache.Arrow/Arrays/ArrayData.cs | Adds SliceShared implementation (retains buffers only). |
| src/Apache.Arrow/Arrays/Array.cs | Adds Array.SliceShared API. |
Comments suppressed due to low confidence (1)
src/Apache.Arrow/Memory/SharedMemoryHandle.cs:35
- After
Dispose(),_owneris set to null, butMemory/Retain()dereference_ownerwithout a guard. This turns post-dispose misuse into aNullReferenceExceptionrather than anObjectDisposedException, and it can also surface viaArrowBuffer.Memoryif anArrowBufferis accessed after disposal. Consider adding an explicit disposed check (throwingObjectDisposedException) inMemoryandRetain().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
adamreeve
left a comment
There was a problem hiding this comment.
This looks pretty good to me thanks Curt, I've left a few questions.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
What's Changed
Introduces a reference-counted layer between ArrowBuffer and the underlying memory to allow buffers to be shared between multiple arrays.
Supports export of managed buffers.
Disables experimental workaround that was previously added to support buffer export given that it is no longer necessary.
This is an alternative to #291 that's more flexible.
Closes#111.