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

Add ParseValue methods to JsonElement #42755

Closed
steveharter opened this issue Sep 25, 2020 · 5 comments · Fixed by #43601
Closed

Add ParseValue methods to JsonElement #42755

steveharter opened this issue Sep 25, 2020 · 5 comments · Fixed by #43601
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Milestone

Comments

@steveharter
Copy link
Member

steveharter commented Sep 25, 2020

Background and Motivation

The System.Text.Json.JsonElement is intended to be obtained from a parent JsonDocument which contains a binary "database" and creates the elements from the database as the document is navigated. When the document is done being used, the Dispose method is called to release the pooled memory allocated for the database.

This works fine when the caller can use the Dispose pattern, however there are cases where Dispose can't be used including usages by the deserializer where property or element types of System.Object create a JsonElement (since the actual type isn't known) and assign it to the property\element.

So to work around the Dispose issue, the serializer does this:

using (JsonDocument document = JsonDocument.ParseValue(ref reader))
{
    return document.RootElement.Clone();	
}

This is not intuitive and is slow since the document and element have no hint that the Dispose pattern is not needed and can't apply optimizations such as avoiding some unnecessary allocs and adding caching for literal values True, False and Null.

Proposed API

Currently there are no "Parse" methods on JsonElement (must go through JsonDocument). So this exposes a ParseValue method that can have optimizations applied. Also, since JsonElement does not implement the Dispose pattern, the usage and memory behavior is more intuitive.

namespace System.Text.Json
{
    public struct JsonElement
    {
        +public static JsonElement ParseValue(ref Utf8JsonReader reader);
        +public static bool TryParseValue(ref Utf8JsonReader reader, [NotNullWhen(true)] out JsonElement? element);
    }
}

No other Parse overloads are provided here since the known scenarios only include the serializer. Here's some possible additions based on JsonDocument:

        public static JsonElement Parse(System.Buffers.ReadOnlySequence<byte> utf8Json, JsonElementOptions options);
        public static JsonElement Parse(System.IO.Stream utf8Json, JsonElementOptions options);
        public static JsonElement Parse(System.ReadOnlyMemory<byte> utf8Json, JsonElementOptions options);
        public static JsonElement Parse(System.ReadOnlyMemory<char> json, JsonElementOptions options);
        public static JsonElement Parse(string json, JsonElementOptions options);
        public static Task<JsonElement> ParseAsync(Stream utf8Json, JsonElementOptions options, CancellationToken cancellationToken));

Usage Examples

The serializer would use it like:

        public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
        {
            return JsonElement.ParseValue(ref reader);
        }
@steveharter steveharter added area-System.Text.Json api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 25, 2020
@steveharter steveharter added this to the 6.0.0 milestone Sep 25, 2020
@steveharter steveharter self-assigned this Sep 25, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Sep 25, 2020
@jeffschwMSFT jeffschwMSFT removed the untriaged New issue has not been triaged by the area owner label Sep 25, 2020
@ahsonkhan
Copy link
Contributor

since the document and element have no hint that the Dispose pattern is not needed and can't apply optimizations such as avoiding allocs

I am not sure what types of optimizations and avoiding allocations you are referring to here.
I would be curious to see a proof-of-concept implementation of this because I am having difficulty seeing how such APIs fit into the existing overall implementation and design.

If the primary purpose of this is as an optimization for the serializer, consider starting with an internal-only API. In the meantime, is there any reason to make it public?

cc @bartonjs

@steveharter
Copy link
Member Author

steveharter commented Sep 28, 2020

I am not sure what types of optimizations and avoiding allocations you are referring to here.
I would be curious to see a proof-of-concept implementation of this because I am having difficulty seeing how such APIs fit into the existing overall implementation and design.

Please see the PR.

In the meantime, is there any reason to make it public?

Two reasons:

  1. The linked PR may be implemented in 6.0 out-of-band in a new assembly to avoid an assembly reference to System.Linq.Expressions.dll from the current STJ.dll, so having it public is necessary there (unless we call via reflection, which is not good). If we can get linking working properly and remove the reference, we may not need this new assembly.
  2. Since the serializer needs it, there are likely others who need it. Basically, most users that need to call "Clone" today (which was also added for the serializer).

@ahsonkhan
Copy link
Contributor

ahsonkhan commented Oct 1, 2020

It also adds WriteTo for symmetry.

+public void WriteTo(Utf8JsonWriter writer);

We already have this API:
https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonelement.writeto?view=netcore-3.1

  1. The linked PR may be implemented in 6.0 out-of-band in a new assembly to avoid an assembly reference to System.Linq.Expressions.dll from the current STJ.dll, so having it public is necessary there (unless we call via reflection, which is not good). If we can get linking working properly and remove the reference, we may not need this new assembly.

Can you point me to where in your linked PR the dependency on S.Linq.E is getting introduced? I am surprised by this and would like to understand why we have it.

@steveharter
Copy link
Member Author

We already have this API: (WriteTo)

Yep. Thanks. Updated description.

For S.Linq.E dependency, that is part of supporting the "dynamic" functionality that we want in 6.0. For 5.0 I have a PR out as a code example: #42097 which would leverage the proposed ParseValue API for performance.

@steveharter steveharter changed the title Add ParseValue and WriteTo methods to JsonElement Add ParseValue methods to JsonElement Oct 6, 2020
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Oct 6, 2020
@terrajobst
Copy link
Contributor

terrajobst commented Oct 6, 2020

Video

  • Looks good as proposed
namespace System.Text.Json
{
    public partial struct JsonElement
    {
        public static JsonElement ParseValue(ref Utf8JsonReader reader);
        public static bool TryParseValue(ref Utf8JsonReader reader, [NotNullWhen(true)] out JsonElement? element);
    }
}

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants