Improve ArraySegmentStream with ReadOnlyMemory/Span signatures, avoid extra alloc/copy #2556
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2556 +/- ##
==========================================
+ Coverage 54.55% 54.80% +0.25%
==========================================
Files 335 341 +6
Lines 64749 65941 +1192
Branches 13292 13638 +346
==========================================
+ Hits 35325 36142 +817
- Misses 25594 25897 +303
- Partials 3830 3902 +72 ☔ View full report in Codecov by Sentry. |
|
will add a few benchmarks to understand the impact of the change |
|
TODO: implement decoder benchmarks and GetSequence, to allow wider use of ArraySegmentStream class. |
| /// <summary> | ||
| /// A class to hold a sequence of buffers until disposed. | ||
| /// </summary> | ||
| public class BufferSequence : IDisposable |
There was a problem hiding this comment.
Make final classes sealed for extra perf opportunity for compiler.
| } | ||
| m_sequence = ReadOnlySequence<byte>.Empty; | ||
| m_firstSegment = null; | ||
| GC.SuppressFinalize(this); |
There was a problem hiding this comment.
you mean the whole dispose or just GC.Suppressfinalize? I want to have consistent behavior across the various MemoryStream implementations, so this returns the buffers if a user may have just called ToArray without transferring the buffers or if an exception occurs during the encoding, dispose will return the buffers.
There was a problem hiding this comment.
I was just thinking the GC.SuppressFinalize.
There was a problem hiding this comment.
memory can be reclaimed earlier by the GC, thats my understanding.
Proposed changes
Many
MemoryStreamwriter functions viaBinaryWritercall the byte[] methods with an additional copy and allocation that can be avoided by providing the new methods withint Read(Span<byte> buffer)andvoid Write(ReadOnlySpan<byte> buffer).ArraySegmentStreamoutperforms or is on parity withRecyclableMemoryStreamfor both speed and memory allocations due to the special buffer handling.Current benchmark results:
ArraySegmentStreamgets a speed up by a factor of two with the used test set and lesser memory allocations because the callers in the underlying memory stream do not have to do an extra Rent/copy for small items (inBinaryWriter) like an int when calling the new Write signature (compare BinaryEncoderArraySegmentStream vs BinaryEncoderArraySegmentStreamNoSpan)StreamWriterclass. There is no obvious perf difference. But for the JSON caseRecyclableMemoryStreamoutperforms all other MemoryStream implementations by some 10 or 20%.RecyclableMemoryStreamis for the binary encoder as slow as the NoSpan version of theArraySegmentStream. So maybe the span support is missing? (Checked that it is implemented, needs investigation). For JSON encoding there is no difference between ArraySegmentStream and RecyclableMemoryStream.Additional changes:
Related Issues
Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that apply. You can also fill these out after creating the PR.Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
For references the JSON Encoder results for 1024 buffer size of the stream writer: