Implement 64-bit integer based replacements for .NET memory types #2

anreetka · 2024-10-30T13:27:45Z

This PR implements 64-bit integer based replacements for .NET memory types (Span, ReadOnlySpan, ReadOnlyMemory, Memory, MemoryManager and MemoryOwner)

tolleybot · 2024-10-30T15:01:19Z

csharp/src/Apache.Arrow/Memory/LongMemory.cs

+            }
+        }
+
+        public void CopyTo(LongMemory<T> destination)


This will be inefficient with large datasets. What about using Buffer.MemoryCopy or a bulk copy method to optimize performance?

tolleybot · 2024-10-30T15:08:08Z

csharp/src/Apache.Arrow/Memory/LongMemory.cs

+    {
+        private readonly T[] _array;
+        private readonly long _length;
+


One thought is you could look into using .Net's ArrayPool to mitigate large object allocation. ArrayPool provides reusable arrays, reducing the overhead of frequent allocations and deallocations in memory-intensive applications. It works by pooling arrays of various sizes and reusing them.

Nice article: https://medium.com/@epeshk/the-big-performance-difference-between-arraypools-in-net-b25c9fc5e31d

adamreeve · 2024-10-30T15:56:34Z

csharp/src/Apache.Arrow/Memory/LongMemory.cs

+{
+    internal readonly struct LongMemory<T>
+    {
+        private readonly T[] _array;


This type probably needs to hold a raw pointer rather than an array, arrays in .NET are limited to 2 GiB, which is why the new long types are needed. It might be worth reaching out to Curt who's the Arrow maintainer to say you're looking into this and asking if it's possible for him to share the work he started on.

(apache#38086 (comment))

Actually, thinking about this some more, rather than a raw pointer it probably needs to hold a LongMemoryManager or ILongMemoryOwner, as you need to be able to Pin this and ensure the underlying memory isn't garbage collected. The memory manager is then the thing that might hold a pointer, but here should just be an abstract base class like MemoryManager

adamreeve · 2024-10-30T16:59:24Z

csharp/src/Apache.Arrow/Memory/LongMemory.cs

+            }
+        }
+
+        public Span<T> Span


This should probably return a LongSpan, not sure if the name should change too.

adamreeve · 2024-10-30T17:02:23Z

csharp/src/Apache.Arrow/Memory/LongSpan.cs

+
+        public Span<T> AsSpan()
+        {
+            return _memory.Span;


Getting a Span from a LongSpan should require checking that the length is less than int.MaxValue, and throw an exception if not.

adamreeve · 2024-10-30T17:05:48Z

csharp/src/Apache.Arrow/Memory/LongSpan.cs

+{
+    internal readonly struct LongSpan<T>
+    {
+        private readonly LongMemory<T> _memory;


It might be a good idea to look at how the existing Span type and other corresponding types are implemented in .NET. Eg. if you go to the docs for the Span type (https://learn.microsoft.com/en-us/dotnet/api/system.span-1?view=net-8.0) there's a "Source" link that takes you to https://github.com/dotnet/runtime/blob/5535e31a712343a63f5d7d796cd874e563e5ac14/src/libraries/System.Private.CoreLib/src/System/Span.cs

There you can see Span holds:

internal readonly ref T _reference; private readonly int _length;

I imagine that LongSpan should then look like:

internal readonly ref T _reference; private readonly long _length;

(and so LongSpan will need to be a ref struct )

adamreeve · 2024-10-30T17:17:48Z

csharp/src/Apache.Arrow/Memory/LongMemoryOwner.cs

+
+namespace Apache.Arrow.Memory
+{
+    internal class LongMemoryOwner<T>: IMemoryOwner<T>


I don't think we want to define a LongMemoryOwner class, but rather an ILongMemoryOwner interface similar to IMemoryOwner

adamreeve · 2024-11-07T03:55:20Z

csharp/src/Apache.Arrow/Memory/LargeMemory.cs

+        private IntPtr _ptr;
+        private long _size;
+
+        public LargeMemory(long size)


I think we should try to make these types match the standard, non-large types as closely as possible. Eg. you can see the source for System.Memory here. In .NET the Memory<T> type doesn't have a constructor that will allocate memory, and it isn't disposable. It's a struct that holds a reference to an array or a MemoryManager. I think for simplicity we can skip supporting an array backed LargeMemory and only work with a LargeMemoryManager, so LargeMemory should be quite simple and mostly delegate functionality to a LargeMemoryManager.

The MemoryManager is the thing that is disposable and can actually manage the memory, but this is an abstract class so it doesn't implement memory allocation or disposal directly. In Arrow there's a NativeMemoryManager, which inherits from MemoryManager, and I think a lot of the functionality currently in this LargeMemory class should be in a new LargeNativeMemoryManager class instead, which should look pretty similar to the NativeMemoryManager.

Arrow additionally separates the functionality for allocating memory from the MemoryManager, eg. with the NativeMemoryAllocator class.

In order to integrate this new large memory functionality with Arrow, we might want to do something like extend the MemoryAllocator abstract base class by adding a new method, public ILargeMemoryOwner<byte> AllocateLarge(long length). Then the existing NativeMemoryAllocator could be extended to create either a NativeMemoryManager or LargeNativeMemoryManager depending on which method is used.

…etka/arrow into feat/add-larger-buffer-size

adamreeve

Nice progress Anreet, I've added some comments on the latest changes (some of it just repeating what we discussed in the meeting)

adamreeve · 2024-11-14T00:15:50Z

csharp/src/Apache.Arrow/Memory/ILargeNativeAllocationOwner.cs

+
+namespace Apache.Arrow.Memory
+{
+    internal interface ILargeNativeAllocationOwner


I don't think we need a separate interface here, we can just add a new overload of the Release method to INativeAllocationOwner.

adamreeve · 2024-11-14T00:16:59Z

csharp/src/Apache.Arrow/Memory/ILargeOwnableAllocation.cs

+
+namespace Apache.Arrow.Memory
+{
+    internal interface ILargeOwnableAllocation


I don't think this class is needed yet, let's remove it and we can always add it back later if needed when looking at C Data Interface support.

adamreeve · 2024-11-14T00:17:31Z

csharp/src/Apache.Arrow/Memory/LargeImportedAllocationOwner.cs

+
+namespace Apache.Arrow.Memory
+{
+    internal abstract class LargeImportedAllocationOwner:ILargeNativeAllocationOwner


As above, we can remove this for now, it's only needed for the C Data Interface.

adamreeve · 2024-11-14T00:19:56Z

csharp/src/Apache.Arrow/Memory/LargeMemoryAllocator.cs

+
+namespace Apache.Arrow.Memory
+{
+    internal abstract class LargeMemoryAllocator


I think rather than a separate class, we should add methods for large memory allocation to the existing MemoryAllocator class.

adamreeve · 2024-11-14T00:21:17Z

csharp/src/Apache.Arrow/Memory/LargeNativeMemoryAllocator.cs

+
+namespace Apache.Arrow.Memory
+{
+    internal class LargeNativeMemoryAllocator:LargeMemoryAllocator


As mentioned on the LargeMemoryAllocator, this logic should be added to the existing NativeMemoryAllocator class rather than be a separate class.

adamreeve · 2024-11-14T00:22:07Z

csharp/src/Apache.Arrow/Memory/NullMemoryOwner.cs

+
+    internal class LargeNullMemoryOwner: ILargeMemoryOwner<byte>


Instead of a new class, we can make NullMemoryOwner also implement ILargeMemoryOwner<byte>

…e memory allocations

adamreeve

Sorry, just noticed my previous review wasn't submitted, so this doesn't account for the most recent changes

adamreeve · 2024-11-20T17:10:18Z

csharp/src/Apache.Arrow/Memory/NativeLargeMemoryManager.cs

+        public override unsafe Span<byte> GetSpan()
+        {
+            void* ptr = CalculatePointer(0);
+            return new Span<byte>(ptr, _length);


Do we need to check for overflow here? Might also want a GetLargeSpan method.

adamreeve · 2024-11-20T17:19:20Z

csharp/src/Apache.Arrow/Memory/NativeMemoryAllocator.cs

+            // TODO: Should the allocation be moved to NativeMemory?
+
+            long size = length + Alignment;
+            IntPtr ptr = Marshal.AllocHGlobal(size);


AllocHGlobal only accepts an int size, so we'll need to find a way to allocate > max int32. Maybe use NativeMemory.Alloc instead (https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.nativememory.alloc?view=net-8.0#system-runtime-interopservices-nativememory-alloc(system-uintptr))

Actually, looks like we can use NativeMemory.AlignedAlloc and then we don't need to deal with alignment ourselves

adamreeve · 2024-11-20T17:19:54Z

csharp/src/Apache.Arrow/Memory/NativeMemoryAllocator.cs

+            GC.AddMemoryPressure(bytesAllocated);
+
+            // Ensure all allocated memory is zeroed.
+            manager.LargeMemory.Span.Fill(0);


Will need to use a LargeSpan here so we can fill > 2 GB

adamreeve · 2024-11-20T17:31:29Z

csharp/src/Apache.Arrow/Memory/LargeMemoryManager.cs

+
+namespace Apache.Arrow.Memory
+{
+    public abstract class LargeMemoryManager<T>:ILargeMemoryOwner<T>


I think to help with using an ILargeMemoryOwner in ArrowBuffer, it would help if this also implements the IMemoryOwner interface.

Implement 64-bit integer based replacements for .NET memory types

ee277d0

tolleybot reviewed Oct 30, 2024

View reviewed changes

adamreeve reviewed Oct 30, 2024

View reviewed changes

anreetka added 3 commits November 6, 2024 12:18

replaced implementation for LargeMemory and LargeSpan

ac9bbc9

added LargeMemoryOwner implementation

c82b049

fixed ptr type in LargeMemory

5064b9d

anreetka requested a review from adamreeve November 6, 2024 18:06

need to implement LargeMemoryOwner interface

25a5eaf

adamreeve reviewed Nov 7, 2024

View reviewed changes

anreetka added 3 commits November 13, 2024 09:13

implemented ILargeMemoryowner and LargeMemoryManager

80f8672

Merge branch 'feat/add-larger-buffer-size' of https://github.com/anre…

795fa65

…etka/arrow into feat/add-larger-buffer-size

added large types for memory allocation classes and interfaces

56e00e3

adamreeve reviewed Nov 14, 2024

View reviewed changes

anreetka added 2 commits November 20, 2024 11:46

Add ILargeMemoryOwner Interface and related classes for handling larg…

053f6d1

…e memory allocations

added length and offset to LargeStringArray

202093a

adamreeve reviewed Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement 64-bit integer based replacements for .NET memory types #2

Implement 64-bit integer based replacements for .NET memory types #2

anreetka commented Oct 30, 2024

tolleybot Oct 30, 2024

tolleybot Oct 30, 2024

adamreeve Oct 30, 2024

adamreeve Oct 30, 2024

adamreeve Oct 30, 2024 •

edited

Loading

adamreeve Oct 30, 2024

adamreeve Oct 30, 2024

adamreeve Oct 30, 2024 •

edited

Loading

adamreeve Oct 30, 2024

adamreeve Nov 7, 2024

adamreeve left a comment

adamreeve Nov 14, 2024

adamreeve Nov 14, 2024

adamreeve Nov 14, 2024

adamreeve Nov 14, 2024

adamreeve Nov 14, 2024

adamreeve Nov 14, 2024

adamreeve left a comment

adamreeve Nov 20, 2024

adamreeve Nov 20, 2024

adamreeve Nov 20, 2024

adamreeve Nov 20, 2024

adamreeve Nov 20, 2024


		internal class LargeNullMemoryOwner: ILargeMemoryOwner<byte>

Implement 64-bit integer based replacements for .NET memory types #2

Are you sure you want to change the base?

Implement 64-bit integer based replacements for .NET memory types #2

Conversation

anreetka commented Oct 30, 2024

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

adamreeve Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamreeve Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamreeve 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

adamreeve 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

adamreeve Oct 30, 2024 •

edited

Loading

adamreeve Oct 30, 2024 •

edited

Loading