Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JsonSerializer writing into Utf8JsonWriter #29205

Closed
BrennanConroy opened this issue Apr 8, 2019 · 8 comments · Fixed by dotnet/corefx#38869
Closed

JsonSerializer writing into Utf8JsonWriter #29205

BrennanConroy opened this issue Apr 8, 2019 · 8 comments · Fixed by dotnet/corefx#38869
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@BrennanConroy
Copy link
Member

Currently if you want to deserialize an object into a Utf8JsonWriter you need to first parse the object with the serializer, then create a temporary JsonDocument, then write the JsonElement into the Utf8JsonWriter.

There should be a way to do this directly from the serializer and avoid the temporary JsonDocument.

var writer = new Utf8JsonWriter();
var bytes = JsonSerializer.ToBytes(someObject, someObject?.GetType());
using var token = JsonDocument.Parse(bytes);
token.RootElement.WriteAsProperty(someReadOnlyByteName, ref writer);
@JamesNK
Copy link
Member

JamesNK commented Apr 8, 2019

Example of it being used to serialize values into a JSON array:

var writer = new Utf8JsonWriter();
writer.WriteStartArray();
foreach (var item in Items)
{
    JsonSerializer.Write(writer, item);
}
writer.WriteEndArray();

@halter73
Copy link
Member

halter73 commented Apr 9, 2019

Related, but Utf8JsonReader dotnet/corefx#36717

@rynowak
Copy link
Member

rynowak commented May 28, 2019

We came across another need for this when writing a converter. You often want to re-enter the serializer to handle some value inside a converter.

writer.WritePropertyName("Value");
JsonSerializer.Write(writer, value, option);
public static partial class JsonSerializer
{
    public static void Write(object value, Type type, Utf8JsonWriter writer, JsonSerializerOptions options = null) { throw null; }
    public static void Write<TValue>(TValue value, Utf8JsonWriter writer, JsonSerializerOptions options = null) { throw null; }
}

@ahsonkhan
Copy link
Member

It might be easier to review both the read and write side together (https://github.com/dotnet/corefx/issues/36714 / https://github.com/dotnet/corefx/issues/36717), and also in the context of existing APIs:

API Proposal:

public static partial class JsonSerializer
{
    // Where we have a disagreement between the options used to create the reader/writer and the
    // JsonSerializerOptions that are passed in, we will honor the options on the reader/writer.

    // ADD:
    public static object ReadValue(ref Utf8JsonReader reader, Type returnType, JsonSerializerOptions options = null) { throw null; }
    public static TValue ReadValue<TValue>(ref Utf8JsonReader reader, JsonSerializerOptions options = null) { throw null; }
    public static void WriteAsValue(object value, Type type, Utf8JsonWriter writer, JsonSerializerOptions options = null) { throw null; }
    public static void WriteAsValue<TValue>(TValue value, Utf8JsonWriter writer, JsonSerializerOptions options = null) { throw null; }
    public static void WriteAsProperty(ReadOnlySpan<byte> utf8PropertyName, object value, Type type, Utf8JsonWriter writer, JsonSerializerOptions options = null) { throw null; }
    public static void WriteAsProperty<TValue>(ReadOnlySpan<char> propertyName, TValue value, Utf8JsonWriter writer, JsonSerializerOptions options = null) { throw null; }
    public static void WriteAsProperty<TValue>(string propertyName, TValue value, Utf8JsonWriter writer, JsonSerializerOptions options = null) { throw null; }
    public static void WriteAsProperty<TValue>(JsonEncodedText propertyName, TValue value, Utf8JsonWriter writer, JsonSerializerOptions options = null) { throw null; }

    // EXISTING:
    public static object Parse(ReadOnlySpan<byte> utf8Json, Type returnType, JsonSerializerOptions options = null) { throw null; }
    public static object Parse(string json, Type returnType, JsonSerializerOptions options = null) { throw null; }
    public static TValue Parse<TValue>(ReadOnlySpan<byte> utf8Json, JsonSerializerOptions options = null) { throw null; }
    public static TValue Parse<TValue>(string json,JsonSerializerOptions options = null) { throw null; }
    public static ValueTask<object> ReadAsync(Stream utf8Json, Type returnType, JsonSerializerOptions options = null, CancellationToken cancellationToken = default(CancellationToken)) { throw null; }
    public static ValueTask<TValue> ReadAsync<TValue>(Stream utf8Json, JsonSerializerOptions options = null, CancellationToken cancellationToken = default(CancellationToken)) { throw null; }
    public static byte[] ToUtf8Bytes(object value, Type type, JsonSerializerOptions options = null) { throw null; }
    public static byte[] ToUtf8Bytes<TValue>(TValue value, JsonSerializerOptions options = null) { throw null; }
    public static string ToString(object value, Type type, JsonSerializerOptions options = null) { throw null; }
    public static string ToString<TValue>(TValue value, JsonSerializerOptions options = null) { throw null; }
    public static Task WriteAsync(object value, Type type, Stream utf8Json, JsonSerializerOptions options = null, CancellationToken cancellationToken = default(CancellationToken)) { throw null; }
    public static Task WriteAsync<TValue>(TValue value, Stream utf8Json, JsonSerializerOptions options = null, CancellationToken cancellationToken = default(CancellationToken)) { throw null; }
}

Questions:

  1. Parameter ordering. The output sink to write to is not the first parameter (stream/writer), while the input source is (stream/read).
  2. In terms of naming, ReadValue was chosen to match JsonDocument.ParseValue. Should it be just Read?
  3. Should we just have Write APIs, or having both WriteAsValue and WriteAsProperty overloads useful/convenient.

Existing APIs that take Utf8JsonReader/Utf8JsonWriter, for comparison:

public sealed partial class JsonDocument : IDisposable
{
    public static JsonDocument ParseValue(ref Utf8JsonReader reader) { throw null; }
    public static bool TryParseValue(ref Utf8JsonReader reader, out JsonDocument document) { throw null; }
}
public readonly partial struct JsonElement
{
    public void WriteAsProperty(ReadOnlySpan<byte> utf8PropertyName, Utf8JsonWriter writer) { }
    public void WriteAsProperty(ReadOnlySpan<char> propertyName, Utf8JsonWriter writer) { }
    public void WriteAsProperty(string propertyName, Utf8JsonWriter writer) { }
    public void WriteAsValue(Utf8JsonWriter writer) { }

    // ADD:
    public void WriteAsProperty(JsonEncodedText propertyName, Utf8JsonWriter writer) { }
}

Existing APIs taken from the ref, for reference:
https://github.com/dotnet/corefx/blob/e23119d577e644d2c2a25419c88c1181681358e0/src/System.Text.Json/ref/System.Text.Json.cs#L380-L394

@steveharter
Copy link
Member

Parameter ordering. The output sink to write to is not the first parameter (stream/writer), while the input source is (stream/read).

Originally I thought this was fine - the object being written is "more pertinent" than the stream so it should be first. However I see value in having the stream first considering if we ever expect this to be an extension method on Stream.

In terms of naming, ReadValue was chosen to match JsonDocument.ParseValue. Should it be just Read?
Should we just have Write APIs, or having both WriteAsValue and WriteAsProperty overloads useful/convenient.

I prefer Just Read.

Should we just have Write APIs, or having both WriteAsValue and WriteAsProperty overloads useful/convenient.

Assuming the writer will have a pulib API added to write just the property name, at this point I would say we just have Write methods (and not call them WriteAsValue). We can add WriteAsProperty in the future if there is demand for it.

@ahsonkhan
Copy link
Member

ahsonkhan commented May 30, 2019

Assuming the writer will have a pulib API added to write just the property name, at this point I would say we just have Write methods (and not call them WriteAsValue). We can add WriteAsProperty in the future if there is demand for it.

I am working under the assumption we don't have the WritePropertyName API on the writer for a few reasons:

  1. For preview 6, we won't have this API, so not providing the WriteAsValue/Property overloads will make the API not usable. I would wait for the converter design to finish and we can adjust then.
  2. I have investigation pending for the perf impact of adding such an API on the rest of the writer (validation changes, etc.).
  3. I do know that writing property name and value separately is ~10% slower than writing it together, so going off of that, we want to keep these APIs regardless, especially since one of the scenarios is related to performance.

@terrajobst
Copy link
Member

terrajobst commented May 30, 2019

Video

  • We should make the stream, reader, writer-thingie the first argument as they are logically extension methods
  • ReadValue makes sense
  • The As infix notation feels odd. We should just call it WriteValue and WriteProperty. We should also change them in JsonElement.
  • Adding WriteProperty(JsonEncodedText propertyName, Utf8JsonWriter writer) makes sense.

@BrennanConroy
Copy link
Member Author

Here is the perf difference from before and after, looks good!

Method Input HubProtocol Mean Error StdDev Op/s Gen 0 Allocated
WriteSingleMessage FewArguments Json 1,662.9 ns 31.874 ns 37.944 ns 601,349.7 0.0038 176 B
WriteSingleMessage FewArguments Json-Before 4,060.1 ns 80.331 ns 176.33 ns 246,297.2 0.0229 848 B
WriteSingleMessage LargeArguments Json 35,299.4 ns 691.042 ns 1,246.091 ns 28,329.1 0.6409 24040 B
WriteSingleMessage LargeArguments Json-Before 82,787.4 ns 1,629.772 ns 3,401.94 ns 12,079.1 1.3428 48384 B
WriteSingleMessage ManyArguments Json 3,615.5 ns 71.133 ns 133.606 ns 276,587.4 0.0076 376 B
WriteSingleMessage ManyArguments Json-Before 9,234.4 ns 192.030 ns 471.053 ns 108,291.3 0.0610 2184 B
WriteSingleMessage NoArguments Json 374.6 ns 7.371 ns 11.257 ns 2,669,597.2 0.0019 72 B
WriteSingleMessage NoArguments Json-Before 380.7 ns 7.507 ns 11.69 ns 2,626,996.8 0.0019 72 B

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 3.0 milestone Feb 1, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants