From 203b4ff3d66cf9ad75dd6025042de58ec33534ae Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 16 Aug 2021 09:43:12 +0200 Subject: [PATCH 1/4] write a failing test --- .../ReadOnlySequenceFactory.byte.cs | 62 +++++++++++++++++++ .../ReadOnlySequenceTests.byte.cs | 5 ++ 2 files changed, 67 insertions(+) diff --git a/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceFactory.byte.cs b/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceFactory.byte.cs index d2c430505ec56b..04af86e5c9782d 100644 --- a/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceFactory.byte.cs +++ b/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceFactory.byte.cs @@ -4,6 +4,9 @@ using System.Buffers; using System.Collections.Generic; using System.Linq; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Threading; namespace System.Memory.Tests { @@ -11,6 +14,7 @@ public abstract class ReadOnlySequenceFactory { public static ReadOnlySequenceFactory ArrayFactory { get; } = new ArrayTestSequenceFactory(); public static ReadOnlySequenceFactory MemoryFactory { get; } = new MemoryTestSequenceFactory(); + public static ReadOnlySequenceFactory MemoryManagerFactory { get; } = new MemoryManagerTestSequenceFactory(); public static ReadOnlySequenceFactory SingleSegmentFactory { get; } = new SingleSegmentTestSequenceFactory(); public static ReadOnlySequenceFactory SegmentPerItemFactory { get; } = new BytePerSegmentTestSequenceFactory(); public static ReadOnlySequenceFactory SplitInThree { get; } = new SegmentsTestSequenceFactory(3); @@ -112,6 +116,64 @@ public override ReadOnlySequence CreateWithContent(T[] data) } } + internal class MemoryManagerTestSequenceFactory : ReadOnlySequenceFactory + { + public override ReadOnlySequence CreateOfSize(int size) + { +#if DEBUG + return new ReadOnlySequence(new CustomMemoryManager(size + 1).Memory.Slice(1)); // #57472 +#else + return new ReadOnlySequence(new CustomMemoryManager(size).Memory); +#endif + } + + public override ReadOnlySequence CreateWithContent(byte[] data) + { + return new ReadOnlySequence(new CustomMemoryManager(data).Memory); + } + + private unsafe class CustomMemoryManager : MemoryManager + { + private readonly int _size; + private IntPtr _pointer; + private long _referenceCount; + + public CustomMemoryManager(int size) => _pointer = Marshal.AllocHGlobal(_size = size); + + public CustomMemoryManager(byte[] content) + { + _pointer = Marshal.AllocHGlobal(_size = content.Length); + content.AsSpan().CopyTo(new Span(_pointer.ToPointer(), _size)); + } + + public unsafe override Span GetSpan() => new Span(_pointer.ToPointer(), _size); + + public override unsafe MemoryHandle Pin(int elementIndex = 0) + { + if ((uint)elementIndex > (uint)_size) + { + throw new ArgumentOutOfRangeException(nameof(elementIndex)); + } + + Interlocked.Increment(ref _referenceCount); + + return default; // there is no need to pin anything, it's unmanaged memory + } + + public override void Unpin() => Interlocked.Decrement(ref _referenceCount); + + protected override void Dispose(bool disposing) + { + if (Interlocked.Read(ref _referenceCount) == 0) + { + // this code is not thread safe, don't copy and reuse it! + IntPtr pointer = Interlocked.Exchange(ref _pointer, IntPtr.Zero); + Marshal.FreeHGlobal(pointer); + } + } + } + } + public static ReadOnlySequence CreateSegments(params T[][] inputs) => CreateSegments(inputs.Select(input => (ReadOnlyMemory)input.AsMemory())); public static ReadOnlySequence CreateSegments(IEnumerable> inputs) diff --git a/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceTests.byte.cs b/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceTests.byte.cs index eb78a69e2008b2..db53f0834fe9e2 100644 --- a/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceTests.byte.cs +++ b/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceTests.byte.cs @@ -20,6 +20,11 @@ public class Memory : ReadOnlySequenceTestsByte public Memory() : base(ReadOnlySequenceFactory.MemoryFactory) { } } + public class MemoryManager : ReadOnlySequenceTestsByte + { + public MemoryManager() : base(ReadOnlySequenceFactory.MemoryManagerFactory) { } + } + public class SingleSegment : ReadOnlySequenceTestsByte { public SingleSegment() : base(ReadOnlySequenceFactory.SingleSegmentFactory) { } From aff7e307c69d44162e82a86c41b16ed732709a2f Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 16 Aug 2021 10:23:46 +0200 Subject: [PATCH 2/4] change MemoryTestSequenceFactory to test this scenario as well --- .../tests/ReadOnlyBuffer/ReadOnlySequenceFactory.byte.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceFactory.byte.cs b/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceFactory.byte.cs index 04af86e5c9782d..98d7c33ac4b4cc 100644 --- a/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceFactory.byte.cs +++ b/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceFactory.byte.cs @@ -41,7 +41,11 @@ internal class MemoryTestSequenceFactory : ReadOnlySequenceFactory { public override ReadOnlySequence CreateOfSize(int size) { - return CreateWithContent(new T[size]); +#if DEBUG + return new ReadOnlySequence(new ReadOnlyMemory(new T[size + 1]).Slice(1)); // #57472 +#else + return new ReadOnlySequence(new ReadOnlyMemory(new T[size])); +#endif } public override ReadOnlySequence CreateWithContent(T[] data) From c57990d6877dfd8574fbbf4aeb66b1684f71f1fe Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 16 Aug 2021 10:25:45 +0200 Subject: [PATCH 3/4] fix --- .../System.Memory/src/System/Buffers/ReadOnlySequence.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/ReadOnlySequence.cs b/src/libraries/System.Memory/src/System/Buffers/ReadOnlySequence.cs index ff260b6aabbea8..46c124c75f1126 100644 --- a/src/libraries/System.Memory/src/System/Buffers/ReadOnlySequence.cs +++ b/src/libraries/System.Memory/src/System/Buffers/ReadOnlySequence.cs @@ -150,7 +150,7 @@ public ReadOnlySequence(ReadOnlyMemory memory) _startObject = manager; _endObject = manager; _startInteger = ReadOnlySequence.MemoryManagerToSequenceStart(index); - _endInteger = length; + _endInteger = index + length; } else if (MemoryMarshal.TryGetArray(memory, out ArraySegment segment)) { From 07048a348ccc20c1d73f1475bcaa4291fd457834 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 16 Aug 2021 13:03:13 +0200 Subject: [PATCH 4/4] there is no need to use unmanaged memory --- .../ReadOnlySequenceFactory.byte.cs | 51 +++++++------------ 1 file changed, 18 insertions(+), 33 deletions(-) diff --git a/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceFactory.byte.cs b/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceFactory.byte.cs index 98d7c33ac4b4cc..8d00793d6df4fa 100644 --- a/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceFactory.byte.cs +++ b/src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceFactory.byte.cs @@ -14,7 +14,7 @@ public abstract class ReadOnlySequenceFactory { public static ReadOnlySequenceFactory ArrayFactory { get; } = new ArrayTestSequenceFactory(); public static ReadOnlySequenceFactory MemoryFactory { get; } = new MemoryTestSequenceFactory(); - public static ReadOnlySequenceFactory MemoryManagerFactory { get; } = new MemoryManagerTestSequenceFactory(); + public static ReadOnlySequenceFactory MemoryManagerFactory { get; } = new MemoryManagerTestSequenceFactory(); public static ReadOnlySequenceFactory SingleSegmentFactory { get; } = new SingleSegmentTestSequenceFactory(); public static ReadOnlySequenceFactory SegmentPerItemFactory { get; } = new BytePerSegmentTestSequenceFactory(); public static ReadOnlySequenceFactory SplitInThree { get; } = new SegmentsTestSequenceFactory(3); @@ -42,7 +42,7 @@ internal class MemoryTestSequenceFactory : ReadOnlySequenceFactory public override ReadOnlySequence CreateOfSize(int size) { #if DEBUG - return new ReadOnlySequence(new ReadOnlyMemory(new T[size + 1]).Slice(1)); // #57472 + return new ReadOnlySequence(new ReadOnlyMemory(new T[size + 1]).Slice(1)); #else return new ReadOnlySequence(new ReadOnlyMemory(new T[size])); #endif @@ -120,61 +120,46 @@ public override ReadOnlySequence CreateWithContent(T[] data) } } - internal class MemoryManagerTestSequenceFactory : ReadOnlySequenceFactory + internal class MemoryManagerTestSequenceFactory : ReadOnlySequenceFactory { - public override ReadOnlySequence CreateOfSize(int size) + public override ReadOnlySequence CreateOfSize(int size) { #if DEBUG - return new ReadOnlySequence(new CustomMemoryManager(size + 1).Memory.Slice(1)); // #57472 + return new ReadOnlySequence(new CustomMemoryManager(size + 1).Memory.Slice(1)); #else - return new ReadOnlySequence(new CustomMemoryManager(size).Memory); + return new ReadOnlySequence(new CustomMemoryManager(size).Memory); #endif } - public override ReadOnlySequence CreateWithContent(byte[] data) + public override ReadOnlySequence CreateWithContent(T[] data) { - return new ReadOnlySequence(new CustomMemoryManager(data).Memory); + return new ReadOnlySequence(new CustomMemoryManager(data).Memory); } - private unsafe class CustomMemoryManager : MemoryManager + private unsafe class CustomMemoryManager : MemoryManager { - private readonly int _size; - private IntPtr _pointer; - private long _referenceCount; + private readonly T[] _buffer; - public CustomMemoryManager(int size) => _pointer = Marshal.AllocHGlobal(_size = size); + public CustomMemoryManager(int size) => _buffer = new T[size]; - public CustomMemoryManager(byte[] content) - { - _pointer = Marshal.AllocHGlobal(_size = content.Length); - content.AsSpan().CopyTo(new Span(_pointer.ToPointer(), _size)); - } + public CustomMemoryManager(T[] content) => _buffer = content; - public unsafe override Span GetSpan() => new Span(_pointer.ToPointer(), _size); + public unsafe override Span GetSpan() => _buffer; public override unsafe MemoryHandle Pin(int elementIndex = 0) { - if ((uint)elementIndex > (uint)_size) + if ((uint)elementIndex > (uint)_buffer.Length) { throw new ArgumentOutOfRangeException(nameof(elementIndex)); } - Interlocked.Increment(ref _referenceCount); - - return default; // there is no need to pin anything, it's unmanaged memory + var handle = GCHandle.Alloc(_buffer, GCHandleType.Pinned); + return new MemoryHandle(Unsafe.Add((void*)handle.AddrOfPinnedObject(), elementIndex), handle, this); } - public override void Unpin() => Interlocked.Decrement(ref _referenceCount); + public override void Unpin() { } - protected override void Dispose(bool disposing) - { - if (Interlocked.Read(ref _referenceCount) == 0) - { - // this code is not thread safe, don't copy and reuse it! - IntPtr pointer = Interlocked.Exchange(ref _pointer, IntPtr.Zero); - Marshal.FreeHGlobal(pointer); - } - } + protected override void Dispose(bool disposing) { } } }