From: David Fowler Date: Tue, 12 Mar 2019 15:02:02 +0000 (-0700) Subject: Made some tweaks to the BufferSegmentStack (dotnet/corefx#35980) X-Git-Tag: submit/tizen/20210909.063632~11031^2~2205 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=4e32bfa338e1955d41fde653656ee76725a07382;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Made some tweaks to the BufferSegmentStack (dotnet/corefx#35980) - Remove array covariance checks - Added another test for pooled segments Commit migrated from https://github.com/dotnet/corefx/commit/19ec4f3cabcc0796c14f4663c6ee3ab8173ad90d --- diff --git a/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/BufferSegmentStack.cs b/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/BufferSegmentStack.cs index bddf7cb..2e0bb9f 100644 --- a/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/BufferSegmentStack.cs +++ b/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/BufferSegmentStack.cs @@ -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++; } + + /// + /// A simple struct we wrap reference types inside when storing in arrays to + /// bypass the CLR's covariant checks when writing to arrays. + /// + /// + /// We use as a wrapper to avoid paying the cost of covariant checks whenever + /// the underlying array that the 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 + /// + 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 index 0000000..864920a --- /dev/null +++ b/src/libraries/System.IO.Pipelines/tests/BufferSegmentPoolTest.cs @@ -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> 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> 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> GetSegments(ReadResult result) + { + SequenceMarshal.TryGetReadOnlySequenceSegment( + result.Buffer, + out ReadOnlySequenceSegment start, + out int startIndex, + out ReadOnlySequenceSegment end, + out int endIndex); + + var segments = new List>(); + + while (start != end.Next) + { + segments.Add(start); + start = start.Next; + } + + return segments; + } + } +} diff --git a/src/libraries/System.IO.Pipelines/tests/PipeLengthTests.cs b/src/libraries/System.IO.Pipelines/tests/PipeLengthTests.cs index 1448a4e..acc447e 100644 --- a/src/libraries/System.IO.Pipelines/tests/PipeLengthTests.cs +++ b/src/libraries/System.IO.Pipelines/tests/PipeLengthTests.cs @@ -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); diff --git a/src/libraries/System.IO.Pipelines/tests/System.IO.Pipelines.Tests.csproj b/src/libraries/System.IO.Pipelines/tests/System.IO.Pipelines.Tests.csproj index da672ba..5d82180 100644 --- a/src/libraries/System.IO.Pipelines/tests/System.IO.Pipelines.Tests.csproj +++ b/src/libraries/System.IO.Pipelines/tests/System.IO.Pipelines.Tests.csproj @@ -32,6 +32,7 @@ +