Adds PipeWriterAdapter #1065
Adds PipeWriterAdapter #1065
Conversation
Add benchmarks Remove benchmark artifacts Cleanup remove extra stuff nit
benchmarks/Microsoft.AspNetCore.Http.Performance/PipeWriterAdapterBenchmark.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // Get a new buffer using the minimum segment size, unless the size hint is larger than a single segment. | ||
| _currentSegment = ArrayPool<byte>.Shared.Rent(Math.Max(_minimumSegmentSize, sizeHint)); |
There was a problem hiding this comment.
Take IMemoryPool as ctor argument.
There was a problem hiding this comment.
Hmm not sure we want that in the default path.
There was a problem hiding this comment.
ArrayPool<byte>.Shared devirtualizes now dotnet/coreclr#20637 so if you were using IMemoryPool you'd want something like
var size = Math.Max(_minimumSegmentSize, sizeHint);
_currentSegment = _pool?.AllocByte(size) ?? ArrayPool<byte>.Shared.Rent(size);Though, isn't IMemoryPool in the wrong namespace? (i.e. downstream)
There was a problem hiding this comment.
We can start with it and look at benchmark results.
There was a problem hiding this comment.
IMemoryPool allocates like a mofo, we can avoid it in this case with little downside.
There was a problem hiding this comment.
Why don't we do the same in pipe then?
There was a problem hiding this comment.
Or think about the way to fix that allocation
There was a problem hiding this comment.
There are some concrete benefits you get from the IMemoryPool
- Somebody can plug in an implementation (it's an abstraction)
- We have an implementation (in ASP.NET Core) that uses pre-pinned memory (a variant of the first one)
The default implementation (ArrayMemoryPool) allocates a managed object per segment. It's not that big a deal when doing large allocations but can be avoided completely when using the array pool. We get no benefit in ASP.NET Core if these Streams use pre-pinned buffers since the Streams we'll be writing to won't be pinning buffer for async reads. That happens in the transport layer in Kestrel.
Before reverting to use the IMemoryPool, we should get the baseline so we can measure the differences.
| /// <summary> | ||
| /// Implements PipeWriter using a base stream implementation. | ||
| /// </summary> | ||
| public class PipeWriterAdapter : PipeWriter, IDisposable |
There was a problem hiding this comment.
override WriteAsync and provide an optimized path that writes directly to the Stream if there's no buffered data.
There was a problem hiding this comment.
When you say "No buffered data", you mean no data in _completedSegments or _currentSegment?
|
Better perf numbers. BenchmarkDotNet=v0.10.13, OS=Windows 10.0.17134
Intel Xeon CPU E5-1650 v4 3.60GHz, 1 CPU, 12 logical cores and 6 physical cores
.NET Core SDK=2.2.100-preview2-009404
[Host] : .NET Core 2.2.0-preview2-26905-02 (CoreCLR 4.6.26905.03, CoreFX 4.6.26905.02), 64bit RyuJIT
Job-MVJLBU : .NET Core 2.2.0-preview2-26905-02 (CoreCLR 4.6.26905.03, CoreFX 4.6.26905.02), 64bit RyuJIT
Runtime=Core Server=True Toolchain=.NET Core 2.2
RunStrategy=Throughput
|
benchmarks/Microsoft.AspNetCore.Http.Performance/StreamPipeWriterBenchmark.cs
Outdated
Show resolved
Hide resolved
| #else | ||
| #error Target frameworks need to be updated. | ||
| #endif | ||
| segment.Return(); |
There was a problem hiding this comment.
You need to test more cancellation scenarios. Current logic would do crazy things if you continue writing after CancelPendingFlush (you always reset _bytesWritten but clear segments only after writing them all, this might cause double writes/double returns/etc).
There was a problem hiding this comment.
Still adding a few more tests.
|
These numbers don't sit well with me. I don't see how the allocated bytes can be that similar. |
| } | ||
|
|
||
| // Note this currently doesn't do anything. | ||
| // Implementing completions/callbacks would require creating a PipeReaderAdapter. |
There was a problem hiding this comment.
Throw a NotSupportedException
|
What are the latest perf results? |
|
Removed the MemoryStream and made an underlying noop stream instead.
|
| #endif | ||
| _bytesWritten -= segment.Length; | ||
| segment.Return(); | ||
| _completedSegments.RemoveAt(0); |
There was a problem hiding this comment.
For next PR: this is slow and we don't have a benchmark that exercises this code path at all.
|
|
||
| public void Return() | ||
| { | ||
| _memoryOwner.Dispose(); |
There was a problem hiding this comment.
For the next time: null _memoryOwner and add assert to avoid double returns and catch bugs.
Half of dotnet/aspnetcore#3966.
Few questions:
Tests
Performance
Current performance numbers: