Unified OutputWriter and BufferWriter #2163
Conversation
KrzysztofCwalina
force-pushed
the
UnifyWriters
branch
from
d175384 to
a857441
Compare
KrzysztofCwalina
force-pushed
the
UnifyWriters
branch
from
a857441 to
aeeda19
Compare
| } | ||
|
|
||
| public ref struct OutputWriter<T> where T : IBufferWriter<byte> | ||
| public ref partial struct BufferWriter<T> where T : IBufferWriter<byte> |
There was a problem hiding this comment.
Since now it's BufferWriter<T>, the file should be renamed.
There was a problem hiding this comment.
Yeah, I will rename it in a subsequent change, otherwise github gets often confused about the change and treats files are deleted/added.
| { | ||
| _buffered = 0; | ||
| _output = output; | ||
| _span = output.GetSpan(); | ||
| } | ||
|
|
||
| public Span<byte> Span => _span; | ||
| public Span<byte> Buffer => _span; |
There was a problem hiding this comment.
Why Buffer? It would be better to name the property Span like it done in Memory.
There was a problem hiding this comment.
I thought buffer communicates better that the write needs to be Flushed (and one of the fields is called _buffered).
|
|
||
| namespace System.Buffers.Writer | ||
| { | ||
| public ref partial struct BufferWriter<T> where T : IBufferWriter<byte> |
There was a problem hiding this comment.
Why does this struct need to be generic when the T is constrained to only a single type?
There was a problem hiding this comment.
So that IBufferWriter implementations can be structs and be written to without boxing.
|
|
||
| public void Write(string value) | ||
| { | ||
| var utf16Bytes = value.AsSpan().AsBytes(); |
There was a problem hiding this comment.
nit: remove use of var (here and elsewhere)
|
|
||
| public class BufferWriterBench | ||
| { | ||
| private static AsciiString _crlf = "\r\n"; |
There was a problem hiding this comment.
nit: static fields should be prefixed with s_
|
|
||
| static Sink _sink = new Sink(4096); | ||
|
|
||
| const int InnerItterations = 1000000; |
There was a problem hiding this comment.
thanks. will fix
| } | ||
| } | ||
|
|
||
| // copy from System.Buffers.ReaderWriter to isolate cross-dll calls. |
There was a problem hiding this comment.
For some reason source code compiled into the test is a bit faster. I wanted to have totally apples to apples comparison between BufferWriters and so I wanted to have them both in the same project.
cc: @ahsonkhan, @davidfowl, @joshfree
I used the implementation of basic writer logic (fields, Advance, Ensure, etc.) from https://github.com/aspnet/benchmarks/tree/dev/src/PlatformBenchmarks
Added the ability to write ints and IWritables from the removed BufferWriter.
The perf results are super close (probably within the margin of error). Once the writer and Utf8String are in corefx, I think we will be able to switch the ProtformBenchmark implementation to use these.