Skip to content
This repository was archived by the owner on Aug 2, 2023. It is now read-only.

Add support for BufferWriter<T> to the JsonWriter where T is IBufferWriter<byte> #2358

Closed
ahsonkhan wants to merge 9 commits into from
Closed

Add support for BufferWriter<T> to the JsonWriter where T is IBufferWriter<byte> #2358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0f455ed
Add support for BufferWriter<T> to the JsonWriter
ahsonkhan Jun 8, 2018
7f106c8
Fix uses of JsonWriter in perf tests and samples
ahsonkhan Jun 12, 2018
57574db
Code clean up and change EnsureBuffer to return the span.
ahsonkhan Jun 12, 2018
45a103b
Remove extra whitespace.
ahsonkhan Jun 12, 2018
090feac
Merge branch 'master' of https://github.com/dotnet/corefxlab into Add…
ahsonkhan Jun 12, 2018
9c8e666
Remove chatty calls to Flush within the JsonWriter APIs
ahsonkhan Jun 13, 2018
5370b7b
Add Flush API and fix tests.
ahsonkhan Jun 13, 2018
1d26049
Address PR feedback - make JsonWriter generic.
ahsonkhan Jun 13, 2018
e5f7f1b
Revert "Address PR feedback - make JsonWriter generic."
ahsonkhan Jun 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions samples/LowAllocationWebServer/Shared/SampleServer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Threading;
using System.Text.Http.Parser;
using Microsoft.Net;
using System.Buffers.Writer;

namespace LowAllocationWebServer
{
Expand Down Expand Up @@ -62,7 +63,7 @@ static void WriteResponseForGetJson(HttpRequest request, ReadOnlySequence<byte>
response.AppendEoh();

// write response JSON
var jsonWriter = new JsonWriter(response, true, prettyPrint: false);
var jsonWriter = new JsonWriter(new BufferWriter<IBufferWriter<byte>>(response), true, prettyPrint: false);
Copy link
Copy Markdown
Member

@benaadams benaadams Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

var jsonWriter = JsonWriter.Create(BufferWriter.Create(response), true, prettyPrint: false);

jsonWriter.WriteObjectStart();
jsonWriter.WriteArrayStart("values");
for (int i = 0; i < 5; i++)
Expand Down Expand Up @@ -94,7 +95,7 @@ static void WriteResponseForPostJson(HttpRequest request, ReadOnlySequence<byte>
response.AppendEoh();

// write response JSON
var jsonWriter = new JsonWriter(response, true, prettyPrint: false);
var jsonWriter = new JsonWriter(new BufferWriter<IBufferWriter<byte>>(response), true, prettyPrint: false);
Copy link
Copy Markdown
Member

@benaadams benaadams Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

var jsonWriter = JsonWriter.Create(BufferWriter.Create(response), true, prettyPrint: false);

jsonWriter.WriteObjectStart();
jsonWriter.WriteArrayStart("values");
for (int i = 0; i < requestedCount; i++)
Expand Down
7 changes: 1 addition & 6 deletions src/System.Buffers.ReaderWriter/System/Buffers/Writer/BufferWriterT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,7 @@ public void Ensure(int count = 1)
[MethodImpl(MethodImplOptions.NoInlining)]
private void EnsureMore(int count = 0)
{
var buffered = _buffered;
if (buffered > 0)
{
_buffered = 0;
_output.Advance(buffered);
}
Flush();
_span = _output.GetSpan(count);
}

Expand Down
1 change: 1 addition & 0 deletions src/System.Text.JsonLab/System.Text.JsonLab.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,6 @@
</ItemGroup>
<ItemGroup>
<ProjectReference Include="..\System.Text.Formatting\System.Text.Formatting.csproj" />
<ProjectReference Include="..\System.Buffers.ReaderWriter\System.Buffers.ReaderWriter.csproj" />
</ItemGroup>
</Project>
62 changes: 41 additions & 21 deletions src/System.Text.JsonLab/System/Text/Json/JsonWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,30 +3,31 @@

using System.Buffers;
using System.Buffers.Text;
using System.Buffers.Writer;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

namespace System.Text.JsonLab
{
public struct JsonWriter
public ref struct JsonWriter
Copy link
Copy Markdown
Member

@benaadams benaadams Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

public ref struct JsonWriter<TBufferWriter> where TBufferWriter : IBufferWriter<byte>

Copy link
Copy Markdown
Contributor Author

@ahsonkhan ahsonkhan Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help me understand why that would be beneficial?

Also, if we make JsonWriter generic (with the constraint), how can we add support for stream?

Copy link
Copy Markdown
Member

@benaadams benaadams Jun 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help me understand why that would be beneficial?

Two main reasons

  1. If you pass BufferWriter<IBufferWriter<byte>> a struct formatter; it will box to the IBufferWriter<byte> type, allocation and perf issue. Using the generic will avoid the allocation and boxing.
  2. The methods called internally to BufferWriter<T> (e.g. GetSpan and Advance) will go via generic interface dispatch in a shared generic; which can't inline and is also the slowest calling convention for the clr. Changing it to the generic means when you pass it a struct type it will use a non-shared generic and direct, inlinable calls.

Also, if we make JsonWriter generic (with the constraint), how can we add support for stream?

I assume you mean something not currently present?

Make a struct wrapper for Stream and provide a Create overload:

public struct StreamWriter : IBufferWriter<byte>
{
    private Stream _stream;

    public StreamWriter(Stream stream)
    {
        _stream = stream;
    }

    public void Advance(int count)
    {
        // ...
    }

    public Memory<byte> GetMemory(int sizeHint = 0)
    {
        // ...
    }

    public Span<byte> GetSpan(int sizeHint = 0)
    {
        // ...
    }
}

public static class JsonWriter
{
    public static JsonWriter<TBufferWriter> Create<TBufferWriter>(
        TBufferWriter bufferWriter,
        bool isUtf8,
        bool prettyPrint = false)
    where TBufferWriter : IBufferWriter<byte>
    {
        return new JsonWriter<TBufferWriter>(bufferWriter, isUtf8, prettyPrint);
    }

    // Stream overload
    public static JsonWriter<StreamWriter> Create(
        Stream stream,
        bool isUtf8,
        bool prettyPrint = false)
    {
        return new JsonWriter<StreamWriter>(new StreamWriter(stream), isUtf8, prettyPrint);
    }
}

Copy link
Copy Markdown
Member

@MarcoRossignoli MarcoRossignoli Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The methods called internally to BufferWriter (e.g. GetSpan and Advance) will go via generic interface dispatch in a shared generic; which can't inline and is also the slowest calling convention for the clr. Changing it to the generic means when you pass it a struct type it will use a non-shared generic and direct, inlinable calls.

@benaadams is related to CLR behaviour explained here https://blogs.msdn.microsoft.com/joelpob/2004/11/17/clr-generics-and-code-sharing/ ?

Copy link
Copy Markdown
Member

@benaadams benaadams Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The T parameter is explicitly being specified as an interface: IBufferWriter<byte>; so if you create that BufferWriter<IBufferWriter<byte>> and give it a struct as the T, then its boxed to the interface and stored in the BufferWriter<IBufferWriter<byte>> as the boxed IBufferWriter<byte> type

Copy link
Copy Markdown
Contributor Author

@ahsonkhan ahsonkhan Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you mean something not currently present?

Yes.

Make a struct wrapper for Stream and provide a Create overload:

That is a great idea! I was trying to see if there was a way to add stream support directly to JsonWriter without exposing additional public types (like the StreamWriter wrapper). However, the boxing and performance benefits you mentioned are definitely compelling.

Copy link
Copy Markdown
Contributor Author

@ahsonkhan ahsonkhan Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you pass BufferWriter<IBufferWriter<byte>> a struct formatter; it will box to the IBufferWriter<byte> type, allocation and perf issue. Using the generic will avoid the allocation and boxing.

I wasn't seeing any allocations because the formatter used within the benchmarks is a class (ArrayFormatter). Good point.

Copy link
Copy Markdown
Member

@benaadams benaadams Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, missed that. Can always measure the effects of the call costs by creating a second struct that just wraps and using that in a test instead of ArrayFormatter e.g.

public struct ArrayFormatterWrapper : ITextBufferWriter, IDisposable
{
    private ArrayFormatter _arrayFormatter;

    public ArrayFormatter(int capacity, SymbolTable symbolTable, ArrayPool<byte> pool = null)
    {
        _arrayFormatter = new ArrayFormatter(capacity, symbolTable, pool);
    }

    public int CommitedByteCount => _arrayFormatter.CommitedByteCount;

    public void Clear()
    {
        _arrayFormatter.Clear();
    }

    public ArraySegment<byte> Free => _arrayFormatter.Free;
    public ArraySegment<byte> Formatted => _arrayFormatter.Formatted;

    public SymbolTable SymbolTable => _arrayFormatter.SymbolTable;

    public Memory<byte> GetMemory(int minimumLength = 0)
    {
        _arrayFormatter.GetMemory(minimumLength);
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public Span<byte> GetSpan(int minimumLength = 0)
    {
        _arrayFormatter.GetSpan(minimumLength);
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public void Advance(int bytes)
    {
        _arrayFormatter.Advance(bytes);
    }
    public int MaxBufferSize { get; } = _arrayFormatter.MaxBufferSize;

    public void Dispose()
    {
        _arrayFormatter.Dispose();
    }
}

{
private static readonly byte[] s_newLineUtf8 = Encoding.UTF8.GetBytes(Environment.NewLine);
private static readonly char[] s_newLineUtf16 = Environment.NewLine.ToCharArray();

readonly bool _prettyPrint;
readonly IBufferWriter<byte> _bufferWriter;
readonly bool _isUtf8;
private readonly bool _prettyPrint;
private BufferWriter<IBufferWriter<byte>> _bufferWriter;
Copy link
Copy Markdown
Member

@benaadams benaadams Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

private TBufferWriter _bufferWriter;

private readonly bool _isUtf8;

int _indent;
bool _firstItem;
private int _indent;
private bool _firstItem;

/// <summary>
/// Constructs a JSON writer with a specified <paramref name="bufferWriter"/>.
/// </summary>
/// <param name="bufferWriter">An instance of <see cref="ITextBufferWriter" /> used for writing bytes to an output channel.</param>
/// <param name="prettyPrint">Specifies whether to add whitespace to the output text for user readability.</param>
public JsonWriter(IBufferWriter<byte> bufferWriter, bool isUtf8, bool prettyPrint = false)
public JsonWriter(BufferWriter<IBufferWriter<byte>> bufferWriter, bool isUtf8, bool prettyPrint = false)
Copy link
Copy Markdown
Member

@benaadams benaadams Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

public JsonWriter(TBufferWriter bufferWriter, bool isUtf8, bool prettyPrint = false)

{
_bufferWriter = bufferWriter;
_prettyPrint = prettyPrint;
Expand Down Expand Up @@ -1077,14 +1078,18 @@ public void WriteNull()
}
}

public void Flush() => _bufferWriter.Flush();

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void WriteNumberUtf8(long value)
{
//TODO: Optimize, this is too slow
var buffer = _bufferWriter.GetSpan();
Span<byte> buffer = _bufferWriter.Buffer;
int written;
while (!CustomFormatter.TryFormat(value, buffer, out written, JsonConstants.NumberFormat, SymbolTable.InvariantUtf8))
{
buffer = EnsureBuffer();
}

_bufferWriter.Advance(written);
}
Expand All @@ -1093,91 +1098,106 @@ private void WriteNumberUtf8(long value)
private void WriteNumberUtf16(long value)
{
//TODO: Optimize, this is too slow
var buffer = _bufferWriter.GetSpan();
Span<byte> buffer = _bufferWriter.Buffer;
int written;
while (!CustomFormatter.TryFormat(value, buffer, out written, JsonConstants.NumberFormat, SymbolTable.InvariantUtf16))
{
buffer = EnsureBuffer();
}

_bufferWriter.Advance(written);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void WriteNumber(ulong value)
{
var buffer = _bufferWriter.GetSpan();
Span<byte> buffer = _bufferWriter.Buffer;
int written;
while (!CustomFormatter.TryFormat(value, buffer, out written, JsonConstants.NumberFormat, SymbolTable.InvariantUtf8))
{
buffer = EnsureBuffer();
}

_bufferWriter.Advance(written);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void WriteDateTime(DateTime value)
{
var buffer = _bufferWriter.GetSpan();
Span<byte> buffer = _bufferWriter.Buffer;
int written;
while (!CustomFormatter.TryFormat(value, buffer, out written, JsonConstants.DateTimeFormat, SymbolTable.InvariantUtf8))
{
buffer = EnsureBuffer();
}

_bufferWriter.Advance(written);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void WriteDateTimeOffset(DateTimeOffset value)
{
var buffer = _bufferWriter.GetSpan();
Span<byte> buffer = _bufferWriter.Buffer;
int written;
while (!CustomFormatter.TryFormat(value, buffer, out written, JsonConstants.DateTimeFormat, SymbolTable.InvariantUtf8))
{
buffer = EnsureBuffer();
}

_bufferWriter.Advance(written);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void WriteGuid(Guid value)
{
var buffer = _bufferWriter.GetSpan();
Span<byte> buffer = _bufferWriter.Buffer;
int written;
while (!CustomFormatter.TryFormat(value, buffer, out written, JsonConstants.GuidFormat, SymbolTable.InvariantUtf8))
{
buffer = EnsureBuffer();
}

_bufferWriter.Advance(written);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void WriteJsonValueUtf8(ReadOnlySpan<byte> values)
{
var buffer = _bufferWriter.GetSpan();
Span<byte> buffer = _bufferWriter.Buffer;
int written;
while (!SymbolTable.InvariantUtf8.TryEncode(values, buffer, out int consumed, out written))
{
buffer = EnsureBuffer();
}

_bufferWriter.Advance(written);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void WriteJsonValueUtf16(ReadOnlySpan<byte> values)
{
var buffer = _bufferWriter.GetSpan();
Span<byte> buffer = _bufferWriter.Buffer;
int written;
while (!SymbolTable.InvariantUtf16.TryEncode(values, buffer, out int consumed, out written))
{
buffer = EnsureBuffer();
}

_bufferWriter.Advance(written);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void WriteControlUtf8(byte value)
{
MemoryMarshal.GetReference(EnsureBuffer(1)) = value;
Span<byte> buffer = EnsureBuffer(1);
MemoryMarshal.GetReference(buffer) = value;
_bufferWriter.Advance(1);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void WriteControlUtf16(byte value)
{
var buffer = EnsureBuffer(2);
Span<byte> buffer = EnsureBuffer(2);
Unsafe.As<byte, char>(ref MemoryMarshal.GetReference(buffer)) = (char)value;
_bufferWriter.Advance(2);
}
Expand Down Expand Up @@ -1210,7 +1230,7 @@ private void WriteSpacingUtf8(bool newline = true)
var bytesNeeded = newline ? 2 : 0;
bytesNeeded += (indent + 1) * 2;

var buffer = EnsureBuffer(bytesNeeded);
Span<byte> buffer = EnsureBuffer(bytesNeeded);
ref byte utf8Bytes = ref MemoryMarshal.GetReference(buffer);
int idx = 0;

Expand Down Expand Up @@ -1240,7 +1260,7 @@ private void WriteSpacingUtf16(bool newline = true)
bytesNeeded += (indent + 1) * 2;
bytesNeeded *= sizeof(char);

var buffer = EnsureBuffer(bytesNeeded);
Span<byte> buffer = EnsureBuffer(bytesNeeded);
var span = MemoryMarshal.Cast<byte, char>(buffer);
ref char utf16Bytes = ref MemoryMarshal.GetReference(span);
int idx = 0;
Expand All @@ -1263,10 +1283,10 @@ private void WriteSpacingUtf16(bool newline = true)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private Span<byte> EnsureBuffer(int needed = 256)
{
Span<byte> buffer = _bufferWriter.GetSpan(needed);
_bufferWriter.Ensure(needed);
Span<byte> buffer = _bufferWriter.Buffer;
if (buffer.Length < needed)
JsonThrowHelper.ThrowOutOfMemoryException();

return buffer;
}

Expand Down
10 changes: 7 additions & 3 deletions tests/Benchmarks/System.IO.Pipelines/E2E.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Buffers;
using System.Buffers.Text;
using System.Buffers.Writer;
using System.IO.Pipelines.Samples;
using System.Text;
using System.Text.Formatting;
Expand All @@ -29,7 +31,8 @@ public class E2E
[Arguments(10000, 256)]
public void TechEmpowerHelloWorldNoIO(int numberOfRequests, int concurrentConnections)
{
RawInMemoryHttpServer.Run(numberOfRequests, concurrentConnections, s_genericRequest, (request, response) => {
RawInMemoryHttpServer.Run(numberOfRequests, concurrentConnections, s_genericRequest, (request, response) =>
{
var formatter = new BufferWriterFormatter<PipeWriter>(response, SymbolTable.InvariantUtf8);
formatter.Append("HTTP/1.1 200 OK");
formatter.Append("\r\nContent-Length: 13");
Expand All @@ -47,7 +50,8 @@ public void TechEmpowerHelloWorldNoIO(int numberOfRequests, int concurrentConnec
[Arguments(10000, 256)]
public void TechEmpowerJsonNoIO(int numberOfRequests, int concurrentConnections)
{
RawInMemoryHttpServer.Run(numberOfRequests, concurrentConnections, s_genericRequest, (request, response) => {
RawInMemoryHttpServer.Run(numberOfRequests, concurrentConnections, s_genericRequest, (request, response) =>
{
var formatter = new BufferWriterFormatter<PipeWriter>(response, SymbolTable.InvariantUtf8);
formatter.Append("HTTP/1.1 200 OK");
formatter.Append("\r\nContent-Length: 25");
Expand All @@ -57,7 +61,7 @@ public void TechEmpowerJsonNoIO(int numberOfRequests, int concurrentConnections)
formatter.Append("\r\n\r\n");

// write body
var writer = new JsonWriter(formatter, true);
var writer = new JsonWriter(new BufferWriter<IBufferWriter<byte>>(formatter), true, true);
Copy link
Copy Markdown
Member

@benaadams benaadams Jun 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

var writer = new JsonWriter<BufferWriterFormatter<PipeWriter>>(formatter, isUtf8: true, prettyPrint: true);

Perhaps add a convenience static Create

    public static JsonWriter<TBufferWriter> Create<TBufferWriter>(
        TBufferWriter bufferWriter,
        bool isUtf8,
        bool prettyPrint = false)
    where TBufferWriter : IBufferWriter<byte>
    {
        return new JsonWriter<TBufferWriter>(bufferWriter, isUtf8, prettyPrint);
    }

Then instead it would become

var writer = JsonWriter.Create(formatter, isUtf8: true, prettyPrint: true);

Copy link
Copy Markdown
Member

@benaadams benaadams Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed Create method, had it wrong before; it just needs to pass through the generic type

writer.WriteObjectStart();
writer.WriteAttribute("message", "Hello, World!");
writer.WriteObjectEnd();
Expand Down
24 changes: 16 additions & 8 deletions tests/Benchmarks/System.Text.JsonLab/JsonWriterPerf.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using BenchmarkDotNet.Attributes;
using System.Buffers;
using System.Buffers.Text;
using System.Buffers.Writer;
using System.IO;
using System.Text.Formatting;

Expand Down Expand Up @@ -56,10 +58,11 @@ public void Setup()
public void WriterSystemTextJsonBasic()
{
_arrayFormatter.Clear();
var bufferWriter = new BufferWriter<IBufferWriter<byte>>(_arrayFormatter);
if (IsUTF8Encoded)
WriterSystemTextJsonBasicUtf8(Formatted, _arrayFormatter, _data);
WriterSystemTextJsonBasicUtf8(Formatted, bufferWriter, _data);
else
WriterSystemTextJsonBasicUtf16(Formatted, _arrayFormatter, _data);
WriterSystemTextJsonBasicUtf16(Formatted, bufferWriter, _data);
}

[Benchmark]
Expand All @@ -72,10 +75,11 @@ public void WriterNewtonsoftBasic()
public void WriterSystemTextJsonHelloWorld()
{
_arrayFormatter.Clear();
var bufferWriter = new BufferWriter<IBufferWriter<byte>>(_arrayFormatter);
if (IsUTF8Encoded)
WriterSystemTextJsonHelloWorldUtf8(Formatted, _arrayFormatter);
WriterSystemTextJsonHelloWorldUtf8(Formatted, bufferWriter);
else
WriterSystemTextJsonHelloWorldUtf16(Formatted, _arrayFormatter);
WriterSystemTextJsonHelloWorldUtf16(Formatted, bufferWriter);
}

[Benchmark]
Expand All @@ -100,7 +104,7 @@ private TextWriter GetWriter()
return writer;
}

private static void WriterSystemTextJsonBasicUtf8(bool formatted, ArrayFormatter output, int[] data)
private static void WriterSystemTextJsonBasicUtf8(bool formatted, BufferWriter<IBufferWriter<byte>> output, int[] data)
Copy link
Copy Markdown
Member

@benaadams benaadams Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again add a convenience helper

public ref partial struct BufferWriter
{
    public static BufferWriter<T> Create<T>(TBufferWriter bufferWriter) where TBufferWriter : IBufferWriter<byte>
    {
        return new BufferWriter<TBufferWriter>(bufferWriter);
    }
}

Though maybe they should be called BufferedWriter and BufferedWriter<TBufferWriter>?

private static void WriterSystemTextJsonBasicUtf8<TBufferWriter>(bool formatted, TBufferWriter output, int[] data)
    where TBufferWriter : IBufferWriter<byte>
{
    WriterSystemTextJsonBasicUtf8(formatted, BufferWriter.Create(output), data);
}

private static void WriterSystemTextJsonBasicUtf8(bool formatted, BufferWriter<TBufferWriter> output, int[] data)
    where TBufferWriter IBufferWriter<byte>
{
    var  json = JsonWriter.Create(output, isUtf8: true, formatted);
    // ...

Copy link
Copy Markdown
Contributor Author

@ahsonkhan ahsonkhan Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good feedback specific to BufferWriter.
#2177

cc @KrzysztofCwalina, @JeremyKuhne

Copy link
Copy Markdown
Member

@benaadams benaadams Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment for issue

Copy link
Copy Markdown
Member

@benaadams benaadams Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to overload is still relevant for this PR though

Copy link
Copy Markdown
Contributor Author

@ahsonkhan ahsonkhan Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again add a convenience helper

We already have such a convenience method:

corefxlab/src/System.Buffers.ReaderWriter/System/Buffers/Writer/BufferWriter.cs

Lines 24 to 26 in a02d39d

public static BufferWriter<TOutput> Create<TOutput>(TOutput output)
where TOutput : IBufferWriter<byte>
=> new BufferWriter<TOutput>(output);

Copy link
Copy Markdown
Contributor Author

@ahsonkhan ahsonkhan Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also BufferWriter<TBufferWriter> is a ref struct. We can't use it as the generic type in JsonWriter.

Severity Code Description Project File Line Suppression State
Error CS0306 The type BufferWriter<TBufferWriter> may not be used as a type argument

@benaadams, thoughts?

Copy link
Copy Markdown
Member

@benaadams benaadams Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concrete type it, but not its parameter?

public static class JsonWriter
{
    public static JsonWriter<TBufferWriter> Create<TBufferWriter>(
        TBufferWriter bufferWriter,
        bool isUtf8,
        bool prettyPrint = false)
    where TBufferWriter : IBufferWriter<byte>
    {
        return new JsonWriter<TBufferWriter>(BufferWriter.Create(bufferWriter), isUtf8, prettyPrint);
    }

    // Stream overload
    public static JsonWriter<StreamWriter> Create(
        Stream stream,
        bool isUtf8,
        bool prettyPrint = false)
    {
        return new JsonWriter<StreamWriter>(BufferWriter.Create(new StreamWriter(stream)), isUtf8, prettyPrint);
    }
}

public ref struct JsonWriter<TBufferWriter>
    where TBufferWriter : IBufferWriter<byte>
{
    private BufferWriter<TBufferWriter> _bufferWriter;

    public JsonWriter(BufferWriter<TBufferWriter> bufferWriter, bool isUtf8, bool prettyPrint = false)
    {
        _bufferWriter = bufferWriter;
        // ...
    }
}

Copy link
Copy Markdown
Contributor Author

@ahsonkhan ahsonkhan Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concrete type it, but not its parameter?

I don't follow. We still have the issue with JsonWriter.Create since the first argument passed to it is a generic type (TBufferWriter) and bufferWriter is a ref struct:

var formatter = new ArrayFormatter(1024, SymbolTable.InvariantUtf8);
var bufferWriter = BufferWriter.Create(formatter);
var json = JsonWriter.Create(bufferWriter, isUtf8: true, prettyPrint: false); // Build error

Severity Code Description Project File Line Suppression State
Error CS0306 The type BufferWriter<ArrayFormatter> may not be used as a type argument System.Text.JsonLab.Tests E:\GitHub\Fork\corefxlab\tests\System.Text.JsonLab.Tests\JsonWriterTests.cs 23 Active

Copy link
Copy Markdown
Member

@benaadams benaadams Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, gotcha. Just need to add concrete Create overload?

public static class JsonWriter
{
    public static JsonWriter<TBufferWriter> Create<TBufferWriter>(
        TBufferWriter bufferWriter,
        bool isUtf8,
        bool prettyPrint = false)
    where TBufferWriter : IBufferWriter<byte>
    {
        return new JsonWriter<TBufferWriter>(BufferWriter.Create(bufferWriter), isUtf8, prettyPrint);
    }

    // extra overload
    public static JsonWriter<TBufferWriter> Create<TBufferWriter>(
        BufferWriter<TBufferWriter> bufferWriter,
        bool isUtf8,
        bool prettyPrint = false)
    where TBufferWriter : IBufferWriter<byte>
    {
        return new JsonWriter<TBufferWriter>(bufferWriter, isUtf8, prettyPrint);
    }
}

{
var json = new JsonWriter(output, true, formatted);

Expand All @@ -127,9 +131,10 @@ private static void WriterSystemTextJsonBasicUtf8(bool formatted, ArrayFormatter
json.WriteArrayEnd();

json.WriteObjectEnd();
json.Flush();
}

private static void WriterSystemTextJsonBasicUtf16(bool formatted, ArrayFormatter output, int[] data)
private static void WriterSystemTextJsonBasicUtf16(bool formatted, BufferWriter<IBufferWriter<byte>> output, int[] data)
{
var json = new JsonWriter(output, false, formatted);

Expand All @@ -156,6 +161,7 @@ private static void WriterSystemTextJsonBasicUtf16(bool formatted, ArrayFormatte
json.WriteArrayEnd();

json.WriteObjectEnd();
json.Flush();
}

private static void WriterNewtonsoftBasic(bool formatted, TextWriter writer, int[] data)
Expand Down Expand Up @@ -199,22 +205,24 @@ private static void WriterNewtonsoftBasic(bool formatted, TextWriter writer, int
}
}

private static void WriterSystemTextJsonHelloWorldUtf8(bool formatted, ArrayFormatter output)
private static void WriterSystemTextJsonHelloWorldUtf8(bool formatted, BufferWriter<IBufferWriter<byte>> output)
Copy link
Copy Markdown
Member

@benaadams benaadams Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above

{
var json = new JsonWriter(output, true, formatted);

json.WriteObjectStart();
json.WriteAttribute("message", "Hello, World!");
json.WriteObjectEnd();
json.Flush();
}

private static void WriterSystemTextJsonHelloWorldUtf16(bool formatted, ArrayFormatter output)
private static void WriterSystemTextJsonHelloWorldUtf16(bool formatted, BufferWriter<IBufferWriter<byte>> output)
{
var json = new JsonWriter(output, false, formatted);

json.WriteObjectStart();
json.WriteAttribute("message", "Hello, World!");
json.WriteObjectEnd();
json.Flush();
}

private static void WriterNewtonsoftHelloWorld(bool formatted, TextWriter writer)
Expand Down
Loading