Copilot commented on code in PR #304: URL: https://github.com/apache/arrow-dotnet/pull/304#discussion_r3019238976
########## src/Apache.Arrow/Memory/NativeBuffer.cs: ########## @@ -0,0 +1,208 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +using System.Buffers; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Threading; + +namespace Apache.Arrow.Memory +{ + public sealed class NativeBuffer<TItem, TTracker> : IDisposable + where TItem : struct + where TTracker : struct, INativeAllocationTracker + { + private const int Alignment = MemoryAllocator.DefaultAlignment; + + private MemoryManager _owner; + private int _byteLength; + + /// <summary>Number of <typeparamref name="TItem"/> elements that fit in the buffer.</summary> + public int Length { get; private set; } + + /// <summary>Creates a native buffer sized for <paramref name="elementCount"/> elements of <typeparamref name="TItem"/>.</summary> + /// <param name="elementCount">Number of elements.</param> + /// <param name="zeroFill">If true, the buffer is zeroed. Set to false if the caller will initialize the entire span itself.</param> + /// <param name="tracker">Allows native allocation sizes to be tracked and affect the GC.</param> + public NativeBuffer(int elementCount, bool zeroFill = true, TTracker tracker = default) + { + int elementSize = Unsafe.SizeOf<TItem>(); + _byteLength = checked(elementCount * elementSize); + Length = elementCount; + _owner = new MemoryManager(_byteLength, tracker); + + if (zeroFill) + { + Span.Clear(); + } + } + + /// <summary>Gets a <see cref="Span{T}"/> over the native buffer.</summary> + public Span<TItem> Span + { + get + { + var byteSpan = _owner!.Memory.Span; + return MemoryMarshal.Cast<byte, TItem>(byteSpan); + } + } + + /// <summary>Gets a <see cref="Span{T}"/> over the raw bytes of the native buffer.</summary> + public Span<byte> ByteSpan => _owner!.Memory.Span.Slice(0, _byteLength); Review Comment: `Span`/`ByteSpan` use `_owner!` and will currently throw `NullReferenceException` after `Dispose()`/`Build()`. It’s more consistent (and easier to diagnose) to throw `ObjectDisposedException` from these accessors when `_owner` is null, similar to `Build()`/`Grow()` behavior. ```suggestion var owner = _owner ?? throw new ObjectDisposedException(nameof(NativeBuffer<TItem, TTracker>)); var byteSpan = owner.Memory.Span; return MemoryMarshal.Cast<byte, TItem>(byteSpan); } } /// <summary>Gets a <see cref="Span{T}"/> over the raw bytes of the native buffer.</summary> public Span<byte> ByteSpan { get { var owner = _owner ?? throw new ObjectDisposedException(nameof(NativeBuffer<TItem, TTracker>)); return owner.Memory.Span.Slice(0, _byteLength); } } ``` ########## src/Apache.Arrow/Memory/NativeBuffer.cs: ########## @@ -0,0 +1,208 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +using System.Buffers; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Threading; + +namespace Apache.Arrow.Memory +{ + public sealed class NativeBuffer<TItem, TTracker> : IDisposable + where TItem : struct Review Comment: `NativeBuffer<TItem, TTracker>` only constrains `TItem` to `struct`, but the implementation uses `MemoryMarshal.Cast<byte, TItem>` over unmanaged memory. This is unsafe for structs containing references and can throw at runtime. Consider changing the constraint to `where TItem : unmanaged` (and keep `Unsafe.SizeOf<TItem>()`) to make the API safe by construction. ```suggestion where TItem : unmanaged ``` ########## src/Apache.Arrow/Memory/NativeBuffer.cs: ########## @@ -0,0 +1,208 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +using System.Buffers; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Threading; + +namespace Apache.Arrow.Memory +{ + public sealed class NativeBuffer<TItem, TTracker> : IDisposable + where TItem : struct + where TTracker : struct, INativeAllocationTracker + { + private const int Alignment = MemoryAllocator.DefaultAlignment; + + private MemoryManager _owner; + private int _byteLength; + + /// <summary>Number of <typeparamref name="TItem"/> elements that fit in the buffer.</summary> + public int Length { get; private set; } + + /// <summary>Creates a native buffer sized for <paramref name="elementCount"/> elements of <typeparamref name="TItem"/>.</summary> + /// <param name="elementCount">Number of elements.</param> + /// <param name="zeroFill">If true, the buffer is zeroed. Set to false if the caller will initialize the entire span itself.</param> + /// <param name="tracker">Allows native allocation sizes to be tracked and affect the GC.</param> + public NativeBuffer(int elementCount, bool zeroFill = true, TTracker tracker = default) + { + int elementSize = Unsafe.SizeOf<TItem>(); + _byteLength = checked(elementCount * elementSize); + Length = elementCount; + _owner = new MemoryManager(_byteLength, tracker); + + if (zeroFill) + { + Span.Clear(); + } + } + + /// <summary>Gets a <see cref="Span{T}"/> over the native buffer.</summary> + public Span<TItem> Span + { + get + { + var byteSpan = _owner!.Memory.Span; + return MemoryMarshal.Cast<byte, TItem>(byteSpan); + } + } + + /// <summary>Gets a <see cref="Span{T}"/> over the raw bytes of the native buffer.</summary> + public Span<byte> ByteSpan => _owner!.Memory.Span.Slice(0, _byteLength); + + /// <summary> + /// Transfers ownership to an <see cref="ArrowBuffer"/>. This instance becomes unusable. + /// </summary> + public ArrowBuffer Build() + { + var owner = _owner ?? throw new ObjectDisposedException(nameof(NativeBuffer<TItem, TTracker>)); + _owner = null; + return new ArrowBuffer(owner); + } + + /// <summary> + /// Grows the buffer to hold at least <paramref name="newElementCount"/> elements, + /// preserving existing data. + /// </summary> + public void Grow(int newElementCount, bool zeroFill = true) + { + if (newElementCount <= Length) + return; + + // Exponential growth (2x) to amortise repeated grows + // TODO: There might be a size that's big enough to work for this case but not too big to overflow. + // We could use that instead of blindly doubling. + int newCount = Math.Max(newElementCount, checked(Length * 2)); + int elementSize = Unsafe.SizeOf<TItem>(); + int needed = checked(newCount * elementSize); + + var owner = _owner ?? throw new ObjectDisposedException(nameof(NativeBuffer<TItem, TTracker>)); + owner.Reallocate(needed); Review Comment: `Grow` returns early when `newElementCount <= Length` *before* checking whether the buffer has been disposed/built. This means `Grow(0)`/`Grow(Length)` can succeed silently even after `Dispose()`/`Build()`, which is inconsistent with the intended “unusable after Build/Dispose” semantics. Consider checking `_owner` for null at the start of `Grow`, before the no-op return. ########## test/Apache.Arrow.Tests/NativeBufferTests.cs: ########## @@ -0,0 +1,269 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +using System.Reflection; +using Apache.Arrow.Memory; +using Xunit; + +namespace Apache.Arrow.Tests +{ + public class NativeBufferTests + { + [Fact] + public void AllocWriteReadRoundTrip() + { + using var buf = new NativeBuffer<int, NoOpAllocationTracker>(4); + + Assert.Equal(4, buf.Length); + + buf.Span[0] = 10; + buf.Span[1] = 20; + buf.Span[2] = 30; + buf.Span[3] = 40; + + Assert.Equal(10, buf.Span[0]); + Assert.Equal(20, buf.Span[1]); + Assert.Equal(30, buf.Span[2]); + Assert.Equal(40, buf.Span[3]); + } + + [Fact] + public void ZeroFillInitializesToZero() + { + using var buf = new NativeBuffer<long, NoOpAllocationTracker>(8, zeroFill: true); + + for (int i = 0; i < buf.Length; i++) + { + Assert.Equal(0L, buf.Span[i]); + } + } + + [Fact] + public void GrowPreservesExistingData() + { + using var buf = new NativeBuffer<int, NoOpAllocationTracker>(3); + + buf.Span[0] = 100; + buf.Span[1] = 200; + buf.Span[2] = 300; + + buf.Grow(10); + + Assert.True(buf.Length >= 10); + Assert.Equal(100, buf.Span[0]); + Assert.Equal(200, buf.Span[1]); + Assert.Equal(300, buf.Span[2]); + } + + [Fact] + public void GrowWithSmallerOrEqualCountIsNoOp() + { + using var buf = new NativeBuffer<int, NoOpAllocationTracker>(5); + + buf.Span[0] = 42; + + buf.Grow(5); + Assert.Equal(5, buf.Length); + Assert.Equal(42, buf.Span[0]); + + buf.Grow(3); + Assert.Equal(5, buf.Length); + Assert.Equal(42, buf.Span[0]); + } + + [Fact] + public void BuildTransfersOwnershipToArrowBuffer() + { + var buf = new NativeBuffer<int, NoOpAllocationTracker>(4); + buf.Span[0] = 1; + buf.Span[1] = 2; + + ArrowBuffer arrow = buf.Build(); + + Assert.True(arrow.Length > 0); + var span = arrow.Span.CastTo<int>(); + Assert.Equal(1, span[0]); + Assert.Equal(2, span[1]); + } Review Comment: Several tests call `buf.Build()` and then keep the resulting `ArrowBuffer` alive without disposing it. Since this buffer owns unmanaged memory, relying on finalizers can make tests leak memory and become flaky under parallel runs. Prefer disposing the returned `ArrowBuffer` (e.g., `using var arrow = buf.Build();`). ########## src/Apache.Arrow/Memory/AlignedNative.cs: ########## @@ -0,0 +1,118 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +using System.Runtime.InteropServices; + +namespace Apache.Arrow.Memory +{ + /// <summary> + /// Provides aligned memory allocation on downlevel platforms (.NET Framework / netstandard2.0) + /// by P/Invoking <c>_aligned_malloc</c> / <c>_aligned_free</c> / <c>_aligned_realloc</c> from + /// the Universal C Runtime (ucrtbase.dll). Falls back to <see cref="Marshal.AllocHGlobal"/> + /// with manual alignment if the CRT functions are unavailable. + /// </summary> + internal static unsafe class AlignedNative + { + private static readonly bool s_hasCrt = ProbeCrt(); + + public static void* AlignedAlloc(int size, int alignment, out int offset) + { + void* ptr; + if (s_hasCrt) + { Review Comment: `s_hasCrt` is declared `static readonly`, but the tests attempt to toggle it via reflection to force the fallback path. Setting init-only static fields via reflection is runtime-dependent and may not work reliably (and JIT may treat the value as invariant). If the fallback path needs to be testable, consider making this field non-readonly or exposing an internal test hook so tests can deterministically select the fallback branch. ########## src/Apache.Arrow/Memory/NativeBuffer.cs: ########## @@ -0,0 +1,208 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +using System.Buffers; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Threading; + +namespace Apache.Arrow.Memory +{ + public sealed class NativeBuffer<TItem, TTracker> : IDisposable + where TItem : struct + where TTracker : struct, INativeAllocationTracker + { + private const int Alignment = MemoryAllocator.DefaultAlignment; + + private MemoryManager _owner; + private int _byteLength; + + /// <summary>Number of <typeparamref name="TItem"/> elements that fit in the buffer.</summary> + public int Length { get; private set; } + + /// <summary>Creates a native buffer sized for <paramref name="elementCount"/> elements of <typeparamref name="TItem"/>.</summary> + /// <param name="elementCount">Number of elements.</param> + /// <param name="zeroFill">If true, the buffer is zeroed. Set to false if the caller will initialize the entire span itself.</param> + /// <param name="tracker">Allows native allocation sizes to be tracked and affect the GC.</param> + public NativeBuffer(int elementCount, bool zeroFill = true, TTracker tracker = default) + { + int elementSize = Unsafe.SizeOf<TItem>(); + _byteLength = checked(elementCount * elementSize); + Length = elementCount; + _owner = new MemoryManager(_byteLength, tracker); + Review Comment: The constructor accepts negative `elementCount`. Because `_byteLength` is computed with `checked(elementCount * elementSize)`, negative values won’t throw and can flow into the native allocator (on NET6+ the negative `int` becomes a huge `nuint`), potentially attempting an enormous allocation. Please validate `elementCount >= 0` and throw `ArgumentOutOfRangeException` (and similarly validate `newElementCount >= 0` in `Grow`). ########## src/Apache.Arrow/Memory/NativeBuffer.cs: ########## @@ -0,0 +1,208 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +using System.Buffers; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Threading; + +namespace Apache.Arrow.Memory +{ + public sealed class NativeBuffer<TItem, TTracker> : IDisposable + where TItem : struct + where TTracker : struct, INativeAllocationTracker + { + private const int Alignment = MemoryAllocator.DefaultAlignment; + + private MemoryManager _owner; + private int _byteLength; + + /// <summary>Number of <typeparamref name="TItem"/> elements that fit in the buffer.</summary> + public int Length { get; private set; } + + /// <summary>Creates a native buffer sized for <paramref name="elementCount"/> elements of <typeparamref name="TItem"/>.</summary> + /// <param name="elementCount">Number of elements.</param> + /// <param name="zeroFill">If true, the buffer is zeroed. Set to false if the caller will initialize the entire span itself.</param> + /// <param name="tracker">Allows native allocation sizes to be tracked and affect the GC.</param> + public NativeBuffer(int elementCount, bool zeroFill = true, TTracker tracker = default) + { + int elementSize = Unsafe.SizeOf<TItem>(); + _byteLength = checked(elementCount * elementSize); + Length = elementCount; + _owner = new MemoryManager(_byteLength, tracker); + + if (zeroFill) + { + Span.Clear(); + } + } + + /// <summary>Gets a <see cref="Span{T}"/> over the native buffer.</summary> + public Span<TItem> Span + { + get + { + var byteSpan = _owner!.Memory.Span; + return MemoryMarshal.Cast<byte, TItem>(byteSpan); + } + } + + /// <summary>Gets a <see cref="Span{T}"/> over the raw bytes of the native buffer.</summary> + public Span<byte> ByteSpan => _owner!.Memory.Span.Slice(0, _byteLength); + + /// <summary> + /// Transfers ownership to an <see cref="ArrowBuffer"/>. This instance becomes unusable. + /// </summary> + public ArrowBuffer Build() + { + var owner = _owner ?? throw new ObjectDisposedException(nameof(NativeBuffer<TItem, TTracker>)); + _owner = null; + return new ArrowBuffer(owner); + } + + /// <summary> + /// Grows the buffer to hold at least <paramref name="newElementCount"/> elements, + /// preserving existing data. + /// </summary> + public void Grow(int newElementCount, bool zeroFill = true) + { + if (newElementCount <= Length) + return; + + // Exponential growth (2x) to amortise repeated grows + // TODO: There might be a size that's big enough to work for this case but not too big to overflow. + // We could use that instead of blindly doubling. + int newCount = Math.Max(newElementCount, checked(Length * 2)); + int elementSize = Unsafe.SizeOf<TItem>(); + int needed = checked(newCount * elementSize); + + var owner = _owner ?? throw new ObjectDisposedException(nameof(NativeBuffer<TItem, TTracker>)); + owner.Reallocate(needed); + + if (zeroFill) + { + Span.Slice(Length, newCount - Length).Clear(); + } + + _byteLength = needed; + Length = newCount; + } + + public void Dispose() + { + IDisposable disposable = _owner; + _owner = null; + disposable?.Dispose(); + } + + /// <summary> + /// A <see cref="MemoryManager{T}"/> backed by aligned native memory. + /// On .NET 6+ uses <see cref="NativeMemory.AlignedAlloc"/>; on downlevel platforms + /// uses <see cref="AlignedNative"/> (P/Invoke to ucrtbase.dll with fallback). + /// Disposing frees the native memory. + /// </summary> + private sealed class MemoryManager : MemoryManager<byte> + { + private unsafe void* _pointer; +#if !NET6_0_OR_GREATER + private int _offset; +#endif + private int _length; + private int _pinCount; + private TTracker _tracker; + + public unsafe MemoryManager(int length, TTracker tracker) + { + _length = length; +#if NET6_0_OR_GREATER + _pointer = NativeMemory.AlignedAlloc((nuint)length, (nuint)Alignment); +#else + _pointer = AlignedNative.AlignedAlloc(length, Alignment, out _offset); +#endif + _tracker = tracker; + _tracker.Track(1, length); + } + + public override Span<byte> GetSpan() + { + unsafe + { +#if NET6_0_OR_GREATER + return new Span<byte>(_pointer, _length); +#else + return new Span<byte>((void*)((byte*)_pointer + _offset), _length); +#endif + } + } + + public override MemoryHandle Pin(int elementIndex = 0) + { + Interlocked.Increment(ref _pinCount); + unsafe + { +#if NET6_0_OR_GREATER + return new MemoryHandle((byte*)_pointer + elementIndex, pinnable: this); +#else + return new MemoryHandle((byte*)_pointer + _offset + elementIndex, pinnable: this); +#endif + } + } + + public override void Unpin() + { + Interlocked.Decrement(ref _pinCount); + } + + protected override void Dispose(bool disposing) + { + unsafe + { + if (_pointer != null) + { +#if NET6_0_OR_GREATER + NativeMemory.AlignedFree(_pointer); +#else + AlignedNative.AlignedFree(_pointer); +#endif + _pointer = null; + _tracker.Track(-1, -_length); + } + } + } + + /// <summary> + /// Reallocates the native buffer to <paramref name="newLength"/> bytes in place, + /// preserving existing data. Equivalent to <c>_aligned_realloc</c>. + /// </summary> + /// <exception cref="InvalidOperationException">Thrown if the memory is currently pinned.</exception> + public unsafe void Reallocate(int newLength) + { + if (Volatile.Read(ref _pinCount) > 0) + throw new InvalidOperationException( + "Cannot reallocate a NativeMemoryManager that is currently pinned."); Review Comment: The exception message says "Cannot reallocate a NativeMemoryManager..." but the type here is `NativeBuffer<...>.MemoryManager`. Updating the message to the correct type/name would make diagnostics clearer. ```suggestion "Cannot reallocate a native buffer that is currently pinned."); ``` ########## src/Apache.Arrow/Memory/NativeBuffer.cs: ########## @@ -0,0 +1,208 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +using System.Buffers; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Threading; + +namespace Apache.Arrow.Memory +{ + public sealed class NativeBuffer<TItem, TTracker> : IDisposable + where TItem : struct + where TTracker : struct, INativeAllocationTracker + { + private const int Alignment = MemoryAllocator.DefaultAlignment; + + private MemoryManager _owner; + private int _byteLength; + + /// <summary>Number of <typeparamref name="TItem"/> elements that fit in the buffer.</summary> + public int Length { get; private set; } + + /// <summary>Creates a native buffer sized for <paramref name="elementCount"/> elements of <typeparamref name="TItem"/>.</summary> + /// <param name="elementCount">Number of elements.</param> + /// <param name="zeroFill">If true, the buffer is zeroed. Set to false if the caller will initialize the entire span itself.</param> + /// <param name="tracker">Allows native allocation sizes to be tracked and affect the GC.</param> + public NativeBuffer(int elementCount, bool zeroFill = true, TTracker tracker = default) + { + int elementSize = Unsafe.SizeOf<TItem>(); + _byteLength = checked(elementCount * elementSize); + Length = elementCount; + _owner = new MemoryManager(_byteLength, tracker); + + if (zeroFill) + { + Span.Clear(); + } + } + + /// <summary>Gets a <see cref="Span{T}"/> over the native buffer.</summary> + public Span<TItem> Span + { + get + { + var byteSpan = _owner!.Memory.Span; + return MemoryMarshal.Cast<byte, TItem>(byteSpan); + } + } + + /// <summary>Gets a <see cref="Span{T}"/> over the raw bytes of the native buffer.</summary> + public Span<byte> ByteSpan => _owner!.Memory.Span.Slice(0, _byteLength); + + /// <summary> + /// Transfers ownership to an <see cref="ArrowBuffer"/>. This instance becomes unusable. + /// </summary> + public ArrowBuffer Build() + { + var owner = _owner ?? throw new ObjectDisposedException(nameof(NativeBuffer<TItem, TTracker>)); + _owner = null; + return new ArrowBuffer(owner); + } + + /// <summary> + /// Grows the buffer to hold at least <paramref name="newElementCount"/> elements, + /// preserving existing data. + /// </summary> + public void Grow(int newElementCount, bool zeroFill = true) + { + if (newElementCount <= Length) + return; + + // Exponential growth (2x) to amortise repeated grows + // TODO: There might be a size that's big enough to work for this case but not too big to overflow. + // We could use that instead of blindly doubling. + int newCount = Math.Max(newElementCount, checked(Length * 2)); + int elementSize = Unsafe.SizeOf<TItem>(); + int needed = checked(newCount * elementSize); + + var owner = _owner ?? throw new ObjectDisposedException(nameof(NativeBuffer<TItem, TTracker>)); + owner.Reallocate(needed); + + if (zeroFill) + { + Span.Slice(Length, newCount - Length).Clear(); + } + + _byteLength = needed; + Length = newCount; + } + + public void Dispose() + { + IDisposable disposable = _owner; + _owner = null; + disposable?.Dispose(); + } + + /// <summary> + /// A <see cref="MemoryManager{T}"/> backed by aligned native memory. + /// On .NET 6+ uses <see cref="NativeMemory.AlignedAlloc"/>; on downlevel platforms + /// uses <see cref="AlignedNative"/> (P/Invoke to ucrtbase.dll with fallback). + /// Disposing frees the native memory. + /// </summary> + private sealed class MemoryManager : MemoryManager<byte> + { + private unsafe void* _pointer; +#if !NET6_0_OR_GREATER + private int _offset; +#endif + private int _length; + private int _pinCount; + private TTracker _tracker; + + public unsafe MemoryManager(int length, TTracker tracker) + { + _length = length; +#if NET6_0_OR_GREATER + _pointer = NativeMemory.AlignedAlloc((nuint)length, (nuint)Alignment); +#else + _pointer = AlignedNative.AlignedAlloc(length, Alignment, out _offset); +#endif + _tracker = tracker; + _tracker.Track(1, length); + } + + public override Span<byte> GetSpan() + { + unsafe + { +#if NET6_0_OR_GREATER + return new Span<byte>(_pointer, _length); +#else + return new Span<byte>((void*)((byte*)_pointer + _offset), _length); +#endif + } + } + + public override MemoryHandle Pin(int elementIndex = 0) + { + Interlocked.Increment(ref _pinCount); + unsafe + { +#if NET6_0_OR_GREATER + return new MemoryHandle((byte*)_pointer + elementIndex, pinnable: this); +#else + return new MemoryHandle((byte*)_pointer + _offset + elementIndex, pinnable: this); +#endif + } + } + + public override void Unpin() + { + Interlocked.Decrement(ref _pinCount); + } + + protected override void Dispose(bool disposing) + { + unsafe + { + if (_pointer != null) + { +#if NET6_0_OR_GREATER + NativeMemory.AlignedFree(_pointer); +#else + AlignedNative.AlignedFree(_pointer); +#endif + _pointer = null; + _tracker.Track(-1, -_length); + } + } + } Review Comment: `Dispose(bool)` frees the unmanaged memory even if it is currently pinned (`_pinCount > 0`). This can lead to use-after-free if a consumer holds a `MemoryHandle` (e.g., via `ArrowBuffer.Memory.Pin()`) when the buffer is disposed/finalized. `NativeMemoryManager` in this repo throws when disposing while pinned; consider adding the same pinned check here (at least for `disposing == true`). ########## src/Apache.Arrow/Memory/NativeBuffer.cs: ########## @@ -0,0 +1,208 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +using System.Buffers; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Threading; + +namespace Apache.Arrow.Memory +{ + public sealed class NativeBuffer<TItem, TTracker> : IDisposable + where TItem : struct + where TTracker : struct, INativeAllocationTracker + { + private const int Alignment = MemoryAllocator.DefaultAlignment; + + private MemoryManager _owner; + private int _byteLength; + + /// <summary>Number of <typeparamref name="TItem"/> elements that fit in the buffer.</summary> + public int Length { get; private set; } + + /// <summary>Creates a native buffer sized for <paramref name="elementCount"/> elements of <typeparamref name="TItem"/>.</summary> + /// <param name="elementCount">Number of elements.</param> + /// <param name="zeroFill">If true, the buffer is zeroed. Set to false if the caller will initialize the entire span itself.</param> + /// <param name="tracker">Allows native allocation sizes to be tracked and affect the GC.</param> + public NativeBuffer(int elementCount, bool zeroFill = true, TTracker tracker = default) + { + int elementSize = Unsafe.SizeOf<TItem>(); + _byteLength = checked(elementCount * elementSize); + Length = elementCount; + _owner = new MemoryManager(_byteLength, tracker); + + if (zeroFill) + { + Span.Clear(); + } + } + + /// <summary>Gets a <see cref="Span{T}"/> over the native buffer.</summary> + public Span<TItem> Span + { + get + { + var byteSpan = _owner!.Memory.Span; + return MemoryMarshal.Cast<byte, TItem>(byteSpan); + } + } + + /// <summary>Gets a <see cref="Span{T}"/> over the raw bytes of the native buffer.</summary> + public Span<byte> ByteSpan => _owner!.Memory.Span.Slice(0, _byteLength); + + /// <summary> + /// Transfers ownership to an <see cref="ArrowBuffer"/>. This instance becomes unusable. + /// </summary> + public ArrowBuffer Build() + { + var owner = _owner ?? throw new ObjectDisposedException(nameof(NativeBuffer<TItem, TTracker>)); + _owner = null; + return new ArrowBuffer(owner); + } + + /// <summary> + /// Grows the buffer to hold at least <paramref name="newElementCount"/> elements, + /// preserving existing data. + /// </summary> + public void Grow(int newElementCount, bool zeroFill = true) + { + if (newElementCount <= Length) + return; + + // Exponential growth (2x) to amortise repeated grows + // TODO: There might be a size that's big enough to work for this case but not too big to overflow. + // We could use that instead of blindly doubling. + int newCount = Math.Max(newElementCount, checked(Length * 2)); + int elementSize = Unsafe.SizeOf<TItem>(); + int needed = checked(newCount * elementSize); + + var owner = _owner ?? throw new ObjectDisposedException(nameof(NativeBuffer<TItem, TTracker>)); + owner.Reallocate(needed); + + if (zeroFill) + { + Span.Slice(Length, newCount - Length).Clear(); + } + + _byteLength = needed; + Length = newCount; + } + + public void Dispose() + { + IDisposable disposable = _owner; + _owner = null; + disposable?.Dispose(); + } + + /// <summary> + /// A <see cref="MemoryManager{T}"/> backed by aligned native memory. + /// On .NET 6+ uses <see cref="NativeMemory.AlignedAlloc"/>; on downlevel platforms + /// uses <see cref="AlignedNative"/> (P/Invoke to ucrtbase.dll with fallback). + /// Disposing frees the native memory. + /// </summary> + private sealed class MemoryManager : MemoryManager<byte> + { + private unsafe void* _pointer; +#if !NET6_0_OR_GREATER + private int _offset; +#endif + private int _length; + private int _pinCount; + private TTracker _tracker; + + public unsafe MemoryManager(int length, TTracker tracker) + { + _length = length; +#if NET6_0_OR_GREATER + _pointer = NativeMemory.AlignedAlloc((nuint)length, (nuint)Alignment); +#else + _pointer = AlignedNative.AlignedAlloc(length, Alignment, out _offset); +#endif + _tracker = tracker; + _tracker.Track(1, length); + } Review Comment: On NET6+, `NativeMemory.AlignedAlloc` / `AlignedRealloc` can return a null pointer on allocation failure. The current code doesn’t check for null and will later create spans over a null pointer (or lose the original pointer on realloc). Please check the returned pointer and throw `OutOfMemoryException` (and for realloc, avoid overwriting `_pointer` until success). ########## src/Apache.Arrow/Memory/AlignedNative.cs: ########## @@ -0,0 +1,118 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +using System.Runtime.InteropServices; + +namespace Apache.Arrow.Memory +{ + /// <summary> + /// Provides aligned memory allocation on downlevel platforms (.NET Framework / netstandard2.0) + /// by P/Invoking <c>_aligned_malloc</c> / <c>_aligned_free</c> / <c>_aligned_realloc</c> from + /// the Universal C Runtime (ucrtbase.dll). Falls back to <see cref="Marshal.AllocHGlobal"/> + /// with manual alignment if the CRT functions are unavailable. + /// </summary> + internal static unsafe class AlignedNative + { + private static readonly bool s_hasCrt = ProbeCrt(); + + public static void* AlignedAlloc(int size, int alignment, out int offset) + { + void* ptr; + if (s_hasCrt) + { + ptr = UcrtInterop.AlignedMalloc((IntPtr)size, (IntPtr)alignment); + offset = 0; + } + else + { + int length = size + alignment; + IntPtr address = Marshal.AllocHGlobal(length); + offset = (int)(alignment - (address.ToInt64() & (alignment - 1))); + ptr = (void*)address; + } Review Comment: In the fallback allocation path, `offset = alignment - (address & (alignment - 1))` yields `offset == alignment` when `address` is already aligned, wasting an extra `alignment` bytes for every allocation. Consider using a modulo-based calculation so the offset is 0 when already aligned (e.g., `(alignment - (addr & (alignment - 1))) & (alignment - 1)` for power-of-two alignments). ########## test/Apache.Arrow.Tests/NativeBufferTests.cs: ########## @@ -0,0 +1,269 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System; +using System.Reflection; +using Apache.Arrow.Memory; +using Xunit; + +namespace Apache.Arrow.Tests +{ + public class NativeBufferTests + { + [Fact] + public void AllocWriteReadRoundTrip() + { + using var buf = new NativeBuffer<int, NoOpAllocationTracker>(4); + + Assert.Equal(4, buf.Length); + + buf.Span[0] = 10; + buf.Span[1] = 20; + buf.Span[2] = 30; + buf.Span[3] = 40; + + Assert.Equal(10, buf.Span[0]); + Assert.Equal(20, buf.Span[1]); + Assert.Equal(30, buf.Span[2]); + Assert.Equal(40, buf.Span[3]); + } + + [Fact] + public void ZeroFillInitializesToZero() + { + using var buf = new NativeBuffer<long, NoOpAllocationTracker>(8, zeroFill: true); + + for (int i = 0; i < buf.Length; i++) + { + Assert.Equal(0L, buf.Span[i]); + } + } + + [Fact] + public void GrowPreservesExistingData() + { + using var buf = new NativeBuffer<int, NoOpAllocationTracker>(3); + + buf.Span[0] = 100; + buf.Span[1] = 200; + buf.Span[2] = 300; + + buf.Grow(10); + + Assert.True(buf.Length >= 10); + Assert.Equal(100, buf.Span[0]); + Assert.Equal(200, buf.Span[1]); + Assert.Equal(300, buf.Span[2]); + } + + [Fact] + public void GrowWithSmallerOrEqualCountIsNoOp() + { + using var buf = new NativeBuffer<int, NoOpAllocationTracker>(5); + + buf.Span[0] = 42; + + buf.Grow(5); + Assert.Equal(5, buf.Length); + Assert.Equal(42, buf.Span[0]); + + buf.Grow(3); + Assert.Equal(5, buf.Length); + Assert.Equal(42, buf.Span[0]); + } + + [Fact] + public void BuildTransfersOwnershipToArrowBuffer() + { + var buf = new NativeBuffer<int, NoOpAllocationTracker>(4); + buf.Span[0] = 1; + buf.Span[1] = 2; + + ArrowBuffer arrow = buf.Build(); + + Assert.True(arrow.Length > 0); + var span = arrow.Span.CastTo<int>(); + Assert.Equal(1, span[0]); + Assert.Equal(2, span[1]); + } + + [Fact] + public void BuildMakesBufferUnusable() + { + var buf = new NativeBuffer<int, NoOpAllocationTracker>(4); + buf.Build(); + + Assert.Throws<ObjectDisposedException>(() => buf.Grow(10)); + } + + [Fact] + public void DoubleDisposeDoesNotThrow() + { + var buf = new NativeBuffer<int, NoOpAllocationTracker>(4); + buf.Dispose(); + buf.Dispose(); + } + + [Fact] + public void GrowAfterDisposeThrows() + { + var buf = new NativeBuffer<int, NoOpAllocationTracker>(4); + buf.Dispose(); + + Assert.Throws<ObjectDisposedException>(() => buf.Grow(10)); + } + + [Fact] + public void BuildAfterDisposeThrows() + { + var buf = new NativeBuffer<int, NoOpAllocationTracker>(4); + buf.Dispose(); + + Assert.Throws<ObjectDisposedException>(() => buf.Build()); + } + + [Fact] + public void ZeroElementBufferIsValid() + { + using var buf = new NativeBuffer<int, NoOpAllocationTracker>(0); + + Assert.Equal(0, buf.Length); + Assert.Equal(0, buf.ByteSpan.Length); + } + + [Fact] + public void ByteSpanReflectsTypedSize() + { + using var buf = new NativeBuffer<long, NoOpAllocationTracker>(3); + + Assert.Equal(3, buf.Length); + Assert.Equal(3 * sizeof(long), buf.ByteSpan.Length); + } + + [Fact] + public void MemoryPressureTrackerNotifiesGC() + { + // Smoke test: just verify it doesn't throw. + // We can't directly observe GC memory pressure, but we verify the + // alloc/dealloc cycle completes without error. + var buf = new NativeBuffer<byte, MemoryPressureAllocationTracker>(1024); + buf.Span[0] = 0xFF; + buf.Dispose(); + } + +#if !NET6_0_OR_GREATER + /// <summary> + /// Helper that forces <c>AlignedNative.s_hasCrt</c> to the given value via reflection, + /// runs <paramref name="action"/>, then restores the original value. + /// </summary> + private static void WithForcedFallback(Action action) + { + var type = typeof(Apache.Arrow.Memory.AlignedNative); + var field = type.GetField("s_hasCrt", BindingFlags.NonPublic | BindingFlags.Static); + bool original = (bool)field.GetValue(null); + try + { + field.SetValue(null, false); + action(); + } + finally + { + field.SetValue(null, original); Review Comment: `WithForcedFallback` assumes the private field `AlignedNative.s_hasCrt` exists and is settable. If the field is missing/renamed or cannot be set (e.g., due to being init-only), the helper will throw `NullReferenceException`/`FieldAccessException` and fail tests for reasons unrelated to allocation behavior. Consider asserting `field != null` and skipping the fallback tests when the override can’t be applied. ```suggestion if (field == null || field.IsInitOnly) { // If the field is missing or not writable, skip these fallback-specific tests. throw new Xunit.Sdk.SkipException("Unable to force AlignedNative fallback: field 's_hasCrt' is unavailable or read-only."); } bool original = (bool)field.GetValue(null); try { try { field.SetValue(null, false); } catch (FieldAccessException) { // If we cannot set the field (e.g., due to accessibility), skip the tests. throw new Xunit.Sdk.SkipException("Unable to force AlignedNative fallback: cannot set field 's_hasCrt'."); } action(); } finally { try { field.SetValue(null, original); } catch (FieldAccessException) { // If restoration fails due to access restrictions, there's nothing sensible to do; // swallow to avoid masking the original test result. } ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
