Made some tweaks to the BufferSegmentStack (dotnet/corefx#35980)
authorDavid Fowler <davidfowl@gmail.com>
Tue, 12 Mar 2019 15:02:02 +0000 (08:02 -0700)
committerGitHub <noreply@github.com>
Tue, 12 Mar 2019 15:02:02 +0000 (08:02 -0700)
- Remove array covariance checks
- Added another test for pooled segments

Commit migrated from https://github.com/dotnet/corefx/commit/19ec4f3cabcc0796c14f4663c6ee3ab8173ad90d

src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/BufferSegmentStack.cs
src/libraries/System.IO.Pipelines/tests/BufferSegmentPoolTest.cs [new file with mode: 0644]
src/libraries/System.IO.Pipelines/tests/PipeLengthTests.cs
src/libraries/System.IO.Pipelines/tests/System.IO.Pipelines.Tests.csproj

index bddf7cb..2e0bb9f 100644 (file)
@@ -11,12 +11,12 @@ namespace System.IO.Pipelines
 {
     internal struct BufferSegmentStack
     {
-        private BufferSegment[] _array;
+        private SegmentAsValueType[] _array;
         private int _size;
 
         public BufferSegmentStack(int size)
         {
-            _array = new BufferSegment[size];
+            _array = new SegmentAsValueType[size];
             _size = 0;
         }
 
@@ -25,7 +25,7 @@ namespace System.IO.Pipelines
         public bool TryPop(out BufferSegment result)
         {
             int size = _size - 1;
-            BufferSegment[] array = _array;
+            SegmentAsValueType[] array = _array;
 
             if ((uint)size >= (uint)array.Length)
             {
@@ -43,7 +43,7 @@ namespace System.IO.Pipelines
         public void Push(BufferSegment item)
         {
             int size = _size;
-            BufferSegment[] array = _array;
+            SegmentAsValueType[] array = _array;
 
             if ((uint)size < (uint)array.Length)
             {
@@ -64,5 +64,25 @@ namespace System.IO.Pipelines
             _array[_size] = item;
             _size++;
         }
+
+        /// <summary>
+        /// A simple struct we wrap reference types inside when storing in arrays to
+        /// bypass the CLR's covariant checks when writing to arrays.
+        /// </summary>
+        /// <remarks>
+        /// We use <see cref="SegmentAsValueType"/> as a wrapper to avoid paying the cost of covariant checks whenever
+        /// the underlying array that the <see cref="BufferSegmentStack"/> class uses is written to. 
+        /// We've recognized this as a perf win in ETL traces for these stack frames:
+        /// clr!JIT_Stelem_Ref
+        ///   clr!ArrayStoreCheck
+        ///     clr!ObjIsInstanceOf
+        /// </remarks>
+        private readonly struct SegmentAsValueType
+        {
+            private readonly BufferSegment _value;
+            private SegmentAsValueType(BufferSegment value) => _value = value;
+            public static implicit operator SegmentAsValueType(BufferSegment s) => new SegmentAsValueType(s);
+            public static implicit operator BufferSegment(SegmentAsValueType s) => s._value;
+        }
     }
 }
diff --git a/src/libraries/System.IO.Pipelines/tests/BufferSegmentPoolTest.cs b/src/libraries/System.IO.Pipelines/tests/BufferSegmentPoolTest.cs
new file mode 100644 (file)
index 0000000..864920a
--- /dev/null
@@ -0,0 +1,124 @@
+using System;
+using System.Buffers;
+using System.Collections.Generic;
+using System.Runtime.InteropServices;
+using System.Text;
+using System.Threading.Tasks;
+using Xunit;
+
+namespace System.IO.Pipelines.Tests
+{
+    public class BufferSegmentPoolTest : IDisposable
+    {
+        private readonly TestMemoryPool _pool;
+        private readonly Pipe _pipe;
+
+        public BufferSegmentPoolTest()
+        {
+            _pool = new TestMemoryPool();
+            _pipe = new Pipe(new PipeOptions(_pool, readerScheduler: PipeScheduler.Inline, writerScheduler: PipeScheduler.Inline, pauseWriterThreshold: 0, resumeWriterThreshold: 0, useSynchronizationContext: false));
+        }
+
+        public void Dispose()
+        {
+            _pipe.Writer.Complete();
+            _pipe.Reader.Complete();
+            _pool?.Dispose();
+        }
+
+        [Fact]
+        public async Task BufferSegmentsAreReused()
+        {
+            _pipe.Writer.WriteEmpty(_pool.MaxBufferSize);
+            await _pipe.Writer.FlushAsync();
+
+            ReadResult result = await _pipe.Reader.ReadAsync();
+            object oldSegment = result.Buffer.End.GetObject();
+
+            // This should return the first segment
+            _pipe.Reader.AdvanceTo(result.Buffer.End);
+
+            // One block remaining
+            Assert.Equal(0, _pipe.Length);
+
+            // This should use the segment that was returned
+            _pipe.Writer.WriteEmpty(_pool.MaxBufferSize);
+            await _pipe.Writer.FlushAsync();
+
+            result = await _pipe.Reader.ReadAsync();
+            object newSegment = result.Buffer.End.GetObject();
+            _pipe.Reader.AdvanceTo(result.Buffer.End);
+
+            Assert.Same(oldSegment, newSegment);
+
+            Assert.Equal(0, _pipe.Length);
+        }
+
+        [Fact]
+        public async Task BufferSegmentsPooledUpToThreshold()
+        {
+            int blockCount = Pipe.MaxSegmentPoolSize + 1;
+
+            // Write 256 blocks to ensure they get reused
+            for (int i = 0; i < blockCount; i++)
+            {
+                _pipe.Writer.WriteEmpty(_pool.MaxBufferSize);
+            }
+
+            await _pipe.Writer.FlushAsync();
+
+            ReadResult result = await _pipe.Reader.ReadAsync();
+
+            List<ReadOnlySequenceSegment<byte>> oldSegments = GetSegments(result);
+
+            Assert.Equal(blockCount, oldSegments.Count);
+
+            // This should return them all to the segment pool (256 blocks, the last block will be discarded)
+            _pipe.Reader.AdvanceTo(result.Buffer.End);
+
+            for (int i = 0; i < blockCount; i++)
+            {
+                _pipe.Writer.WriteEmpty(_pool.MaxBufferSize);
+            }
+
+            await _pipe.Writer.FlushAsync();
+
+            result = await _pipe.Reader.ReadAsync();
+
+            List<ReadOnlySequenceSegment<byte>> newSegments = GetSegments(result);
+
+            Assert.Equal(blockCount, newSegments.Count);
+
+            _pipe.Reader.AdvanceTo(result.Buffer.End);
+
+            // Assert Pipe.MaxSegmentPoolSize pooled segments
+            for (int i = 0; i < Pipe.MaxSegmentPoolSize; i++)
+            {
+                Assert.Same(oldSegments[i], newSegments[Pipe.MaxSegmentPoolSize - i - 1]);
+            }
+
+            // The last segment shouldn't exist in the new list of segments at all (it should be new)
+            Assert.DoesNotContain(oldSegments[256], newSegments);
+        }
+
+        private static List<ReadOnlySequenceSegment<byte>> GetSegments(ReadResult result)
+        {
+            SequenceMarshal.TryGetReadOnlySequenceSegment(
+                           result.Buffer,
+                           out ReadOnlySequenceSegment<byte> start,
+                           out int startIndex,
+                           out ReadOnlySequenceSegment<byte> end,
+                           out int endIndex);
+
+            var segments = new List<ReadOnlySequenceSegment<byte>>();
+
+            while (start != end.Next)
+            {
+                segments.Add(start);
+                start = start.Next;
+            }
+
+            return segments;
+        }
+    }
+}
index 1448a4e..acc447e 100644 (file)
@@ -126,34 +126,6 @@ namespace System.IO.Pipelines.Tests
         }
 
         [Fact]
-        public async Task BufferSegmentsAreReused()
-        {
-            _pipe.Writer.WriteEmpty(_pool.MaxBufferSize);
-            await _pipe.Writer.FlushAsync();
-
-            ReadResult result = await _pipe.Reader.ReadAsync();
-            object oldSegment = result.Buffer.End.GetObject();
-
-            // This should return the first segment
-            _pipe.Reader.AdvanceTo(result.Buffer.End);
-
-            // One block remaining
-            Assert.Equal(0, _pipe.Length);
-
-            // This should use the segment that was returned
-            _pipe.Writer.WriteEmpty(_pool.MaxBufferSize);
-            await _pipe.Writer.FlushAsync();
-
-            result = await _pipe.Reader.ReadAsync();
-            object newSegment = result.Buffer.End.GetObject();
-            _pipe.Reader.AdvanceTo(result.Buffer.End);
-
-            Assert.Same(oldSegment, newSegment);
-
-            Assert.Equal(0, _pipe.Length);
-        }
-
-        [Fact]
         public async Task PooledSegmentsDontAffectLastExaminedSegment()
         {
             _pipe.Writer.WriteEmpty(_pool.MaxBufferSize);
index da672ba..5d82180 100644 (file)
@@ -32,6 +32,7 @@
   </ItemGroup>
   <ItemGroup Condition="'$(TargetGroup)' != 'netstandard'">
     <Compile Include="PipeLengthTests.cs" />
+    <Compile Include="BufferSegmentPoolTest.cs" />
     <Compile Include="PipeReaderWriterFacts.nonnetstandard.cs" />
     <Compile Include="PipeResetTests.nonnetstandard.cs" />
     <Compile Include="PipePoolTests.nonnetstandard.cs" />