Add interop between serializer and DOMs #56112

steveharter · 2021-07-21T22:18:17Z

resolves #31274

Verified 100% code coverage and increased coverage for public API validation in other areas including JsonSerializer APIs that use Stream.

dotnet-issue-labeler · 2021-07-21T22:18:22Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-07-21T22:18:25Z

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

resolves #31274

Verified 100% code coverage and increased coverage for public API validation in other areas including JsonSerializer APIs that use Stream.

Author:	steveharter
Assignees:	steveharter
Labels:	`area-System.Text.Json`
Milestone:	6.0.0

eiriktsarpalis · 2021-07-22T08:32:24Z

src/libraries/System.Text.Json/ref/System.Text.Json.cs

@@ -240,6 +258,24 @@ public static partial class JsonSerializer
        public static System.Threading.Tasks.Task SerializeAsync<TValue>(System.IO.Stream utf8Json, TValue value, System.Text.Json.JsonSerializerOptions? options = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
        public static System.Threading.Tasks.Task SerializeAsync<TValue>(System.IO.Stream utf8Json, TValue value, System.Text.Json.Serialization.Metadata.JsonTypeInfo<TValue> jsonTypeInfo, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
        [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or make sure all of the required types are preserved.")]
+        public static System.Text.Json.JsonDocument SerializeToDocument(object? value, System.Type inputType, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }


Shouldn't the SerializetTo* methods also come in async variants?

Yes we could have added async; it wasn't part of the ask. The approved list of API permutations were already greater than originally expected.

Today, JsonDocument only has ParseAsync() (no serialize behavior) so the workaround for that is something like:

Stream stream = ... // Serialize object to stream await JsonSerializer.SerializeAsync(stream, myObject); // Deserialize stream to document JsonDocument doc = await JsonDocument.ParseAsync(stream);

eiriktsarpalis · 2021-07-22T09:41:39Z

src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.cs

@@ -23,7 +23,13 @@ public sealed partial class JsonDocument : IDisposable
    {
        private ReadOnlyMemory<byte> _utf8Json;
        private MetadataDb _parsedData;
-        private byte[]? _extraRentedBytes;
+
+        private byte[]? _extraRentedArrayPoolBytes;


Why are the extra flags needed? They seem to be equivalent to checking whether the rented references are null.

The existing pattern was thread-safe using Interlocked.Exchange(). Instead of doing two calls to Interlocked.Exchange() (the original one and the new one), a bool was added that will avoid the Interlocked.Exchange() if not necessary.

eiriktsarpalis · 2021-07-22T10:25:02Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Node.cs

+
+            // For performance, we avoid calling JsonSerializer.Deserialize(node) since that will allocate an extra byte[].
+
+            using (var output = new PooledByteBufferWriter(options.DefaultBufferSize))


Nit: could avoid two layers of indentation with using var syntax.

I did clean up the PooledByteBufferWriter, but left the writer's using since it isn't used all the way to the end of the method and I prefer the variable scoping.

eiriktsarpalis · 2021-07-22T10:29:30Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Node.cs

+                    }
+                    else
+                    {
+                        node.WriteTo(writer, options);


I wish there was a way we could avoid writing to an intermediate buffer. At this point it's probably not possible with Utf8JsonReader being a concrete ref struct.

At least we only have one intermediate buffer; the current workarounds (before this new support) would have 2.

In theory, we could add another type of reader (and writer; possibly derived) to avoid this.

layomia

LGTM

layomia · 2021-07-26T18:25:12Z

...ies/System.Text.Json/src/System/Text/Json/Serialization/Converters/Node/JsonNodeConverter.cs

@@ -28,6 +28,7 @@ public override void Write(Utf8JsonWriter writer, JsonNode? value, JsonSerialize
            if (value == null)
            {
                writer.WriteNullValue();
+                // Note the JsonSerializer.Deserialize<T>(JsonNode?) also calls WriteNullValue() for a null + root JsonNode.


Do you have the right serializer method referenced in this comment?

Yes -- the Deserialize() for JsonNode has a special check for null to allow it` to be properly deserialized (since a node is actually nullable, unlike document\element).

layomia · 2021-07-26T18:29:23Z

...ibraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Document.cs

+
+        private static TValue? ReadUsingMetadata<TValue>(JsonDocument document, JsonTypeInfo jsonTypeInfo)
+        {
+            ReadOnlySpan<byte> utf8Json = document.GetRawValue(0, includeQuotes: true).Span;


Could this implementation have called document.RootElement and passed that to ReadUsingMetadata<TValue>(JsonElement element, JsonTypeInfo jsonTypeInfo)?

Sure. Thanks

…bug_tests * origin/main: (274 commits) Disable test ConnectWithCertificateForDifferentName_Throws (dotnet#56456) Update dependencies from https://github.com/mono/linker build 20210726.2 (dotnet#56374) Cleanup disabled test conditions (dotnet#56381) [mono] Add GC unsafe transition to mono_unhandled_exception (dotnet#56380) don't fail the file extraction when we can't set last write time (dotnet#56370) Properly rebuild optimization data when it changes (dotnet#56397) Make open function calls in coreclr EINTR resilient on macOS (dotnet#56403) Fix dependency from EventLog to TraceSource ref (dotnet#56417) Fix comments in asm with JitDiffableDasm=1 (dotnet#56416) Catch TcpClient ctor exceptions in FtpWebRequest.CreateConnectionAsync (dotnet#56379) Add interop between serializer and DOMs (dotnet#56112) Fix type loader not recognizing overridden method (dotnet#56337) Prevent Segfault in EventPipe on disable (dotnet#56104) Update runtimeconfig.json and deps.json paths when these break past the MAX_PATH threshold (dotnet#56224) Use native allocator instead of pinning when decompressing embedded PDB (dotnet#56336) Specify win-x64 as a valid platform in the microsoft-net-runtime-* workloads for iOS/tvOS/MacCatalyst (dotnet#56311) Fix FailFast message formatting race (dotnet#56388) Try to fix finalizer-based async tests (dotnet#56384) Fix MetricsEventSource tests (dotnet#56382) Remove invalid Castle.DynamicProxy.Internal.AbstractInvocation from ILLink descriptor files (dotnet#56392) ...

* origin/main: (95 commits) Disable test ConnectWithCertificateForDifferentName_Throws (dotnet#56456) Update dependencies from https://github.com/mono/linker build 20210726.2 (dotnet#56374) Cleanup disabled test conditions (dotnet#56381) [mono] Add GC unsafe transition to mono_unhandled_exception (dotnet#56380) don't fail the file extraction when we can't set last write time (dotnet#56370) Properly rebuild optimization data when it changes (dotnet#56397) Make open function calls in coreclr EINTR resilient on macOS (dotnet#56403) Fix dependency from EventLog to TraceSource ref (dotnet#56417) Fix comments in asm with JitDiffableDasm=1 (dotnet#56416) Catch TcpClient ctor exceptions in FtpWebRequest.CreateConnectionAsync (dotnet#56379) Add interop between serializer and DOMs (dotnet#56112) Fix type loader not recognizing overridden method (dotnet#56337) Prevent Segfault in EventPipe on disable (dotnet#56104) Update runtimeconfig.json and deps.json paths when these break past the MAX_PATH threshold (dotnet#56224) Use native allocator instead of pinning when decompressing embedded PDB (dotnet#56336) Specify win-x64 as a valid platform in the microsoft-net-runtime-* workloads for iOS/tvOS/MacCatalyst (dotnet#56311) Fix FailFast message formatting race (dotnet#56388) Try to fix finalizer-based async tests (dotnet#56384) Fix MetricsEventSource tests (dotnet#56382) Remove invalid Castle.DynamicProxy.Internal.AbstractInvocation from ILLink descriptor files (dotnet#56392) ...

Add interop between serializer and DOMs

82c50b2

steveharter added the area-System.Text.Json label Jul 21, 2021

steveharter added this to the 6.0.0 milestone Jul 21, 2021

steveharter requested a review from layomia July 21, 2021 22:18

steveharter self-assigned this Jul 21, 2021

steveharter requested review from eiriktsarpalis and jozkee as code owners July 21, 2021 22:18

dotnet-issue-labeler bot added the new-api-needs-documentation label Jul 21, 2021

eiriktsarpalis reviewed Jul 22, 2021

View reviewed changes

eiriktsarpalis approved these changes Jul 22, 2021

View reviewed changes

layomia approved these changes Jul 26, 2021

View reviewed changes

steveharter added 3 commits July 26, 2021 17:21

Add escaping tests and misc refactoring

2945d85

Misc cleanup on using statements

ab5fd10

Fix regression caused by earlier refactoring

3c317fa

steveharter merged commit ddc729f into dotnet:main Jul 28, 2021

steveharter deleted the DomExtensions branch July 28, 2021 01:35

ghost locked as resolved and limited conversation to collaborators Aug 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add interop between serializer and DOMs #56112

Add interop between serializer and DOMs #56112

steveharter commented Jul 21, 2021

dotnet-issue-labeler bot commented Jul 21, 2021

ghost commented Jul 21, 2021

eiriktsarpalis Jul 22, 2021

steveharter Jul 26, 2021

eiriktsarpalis Jul 22, 2021

steveharter Jul 26, 2021

eiriktsarpalis Jul 22, 2021

steveharter Jul 26, 2021

eiriktsarpalis Jul 22, 2021

steveharter Jul 26, 2021 •

edited

Loading

layomia left a comment

layomia Jul 26, 2021

steveharter Jul 26, 2021

layomia Jul 26, 2021

steveharter Jul 26, 2021


		// For performance, we avoid calling JsonSerializer.Deserialize(node) since that will allocate an extra byte[].

		using (var output = new PooledByteBufferWriter(options.DefaultBufferSize))

Add interop between serializer and DOMs #56112

Add interop between serializer and DOMs #56112

Conversation

steveharter commented Jul 21, 2021

dotnet-issue-labeler bot commented Jul 21, 2021

ghost commented Jul 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveharter Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

layomia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveharter Jul 26, 2021 •

edited

Loading