JsonNode trimmability improvements #52934

steveharter · 2021-05-18T20:16:11Z

Also some minor perf refactoring around using custom converters due to having the new per-type Create() methods that allow a converter to be specified at creation time. This improves perf 10%-20% on serialize when there are many instances of known-type JsonValue to serialize.

Verified no regression STJ.dll size in a Blazor app with trimming.

dotnet-issue-labeler · 2021-05-18T20:16:15Z

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-05-18T20:16:18Z

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

Issue Details

Fixes #52773

Also some minor perf refactoring around using custom converters due to having the new per-type Create() methods that allow a converter to be specified at creation time. This improves perf 10%-20% on serialize when there are many instances of known-type JsonValue to serialize.

Verified no regression STJ.dll size in a Blazor app with trimming.

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

eerhardt · 2021-05-18T20:19:40Z

In order to validate this change, you should remove https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Json/src/ILLink/ILLink.Suppressions.xml and ensure there are no ILLink warnings.

eerhardt · 2021-05-18T20:22:45Z

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValue.cs

        /// <param name="options">Options to control the behavior.</param>
        /// <returns>The new instance of the <see cref="JsonValue"/> class that contains the specified value.</returns>
-        public static JsonValue? Create<T>(T? value, JsonNodeOptions? options = null)
+        public static JsonValue? Create<[DynamicallyAccessedMembers(JsonHelpers.MembersAccessedOnRead)]T>(T? value, JsonNodeOptions? options = null)


I thought we were going to mark this as RequiresUnreferencedCode.

Only or in addition to DynamicallyAccessedMembers? I assume the same applies to the Add()

For now, it needs to be in addition to DynamicallyAccessedMembers. But the DynamicallyAccessedMembers will be removed with #52268.

I assume the same applies to the Add()

agreed

layomia · 2021-05-18T22:06:02Z

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValue.cs

            }

-            return new JsonValue<T>(value, options);
+            return new JsonValue<T>(value, jsonTypeInfo, options);


Could we create a new internal type, say JsonValueMetadataBased<T> : JsonValue that has a different implementation of the WriteTo method that doesn't root calls to options-based JsonSerializer methods? On this line, we would create and return an instance of the new type instead.

This is similar to the approach taken for a size-opts friendly JsonContent type: #51544. See JsonContent<T> implementation. This was done so that the new metadata-based System.Net.Http.Json methods do not root all converters/reflection-based code, like done in the existing methods.

I think this approach could help with #52943. From the analysis there:

JsonNodeConverter creates new JsonValue

JsonValue.ToString calls JsonSerializer.Serialize without source gen information

In this model, JsonNodeConverter would instead create a new JsonValueMetadataBased<T> whose WriteTo /ToString implementation does not root existing options-based overloads. We'd have to pass a hand-written JsonTypeInfo<T> instance to the ctor.

Can we just have JsonValueMetadataBased<T> : JsonValue<T> instead? This assumes the the override of WriteTo()\ToString() doesn't call base.WriteTo() or base.ToString() and the linker detects that.

I'll need to prototype this and verify against the object-based repro.

This assumes the the override of WriteTo()\ToString() doesn't call base.WriteTo() or base.ToString() and the linker detects that.

Unfortunately, this doesn't work. The linker will preserve the whole hierarchy of virtual/overrides of classes that are instantiated.

Also I believe ToString() is fine based on previous conversation with @eerhardt

Unfortunately, this doesn't work. The linker will preserve the whole hierarchy of virtual/overrides of classes that are instantiated.

The second commit added two new classes (instead of one) so the safe and unsafe overrides are separated.

layomia

LGTM, with one question

layomia · 2021-05-20T16:20:17Z

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonArray.cs

@@ -109,7 +110,7 @@ public void Add<T>(T? value)
                JsonNode? jNode = value as JsonNode;
                if (jNode == null)
                {
-                    jNode = new JsonValue<T>(value);
+                    jNode = new JsonValueNotTrimmable<T>(value);


So adding a T value to a JSON array is not trim-safe. Should we add a safe version?

There is a safe version that takes JsonNode.

This overload was requested by the Azure SDK team. However, it is optional and could be removed since it does lead to unsafe-for-trimming paths.

eerhardt · 2021-05-20T16:40:25Z

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

@@ -518,7 +518,8 @@ public sealed partial class JsonArray : System.Text.Json.Nodes.JsonNode, System.
        public int Count { get { throw null; } }
        bool System.Collections.Generic.ICollection<System.Text.Json.Nodes.JsonNode?>.IsReadOnly { get { throw null; } }
        public void Add(System.Text.Json.Nodes.JsonNode? item) { }
-        public void Add<T>(T? value) { }
+        [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.")]


This message is a little bit mis-leading since it says "Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or...". There is no overload that takes those types.

eerhardt · 2021-05-20T16:42:49Z

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValueNotTrimmable.cs

+        public JsonValueNotTrimmable(TValue value, JsonNodeOptions? options = null) : base(value, options) { }
+
+        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
+            Justification = "The methods used to create this JsonValue are marked RequiresUnreferencedCode.")]


This isn't true. The ctor of this class needs to be marked as RequiresUnreferencedCode.

I believe that will catch a bug in the JsonObject.Dynamic.cs file where we are creating a JsonValueNotTrimmable, but aren't marking that as RequiresUnreferencedCode.

Thanks, I'll address this and other feedback in a new PR.

eerhardt · 2021-05-20T16:43:24Z

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValue.CreateOverloads.cs

+        /// <summary>
+        ///   Initializes a new instance of the <see cref="JsonValue"/> class that contains the specified value.
+        /// </summary>
+        /// <param name="value">The value to add.</param>


(nit) "The value to add." What is the value being added to?

eerhardt · 2021-05-20T16:48:03Z

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValue.cs

@@ -38,13 +40,46 @@ public abstract partial class JsonValue : JsonNode
                    return null;
                }

-                if (element.ValueKind == JsonValueKind.Object || element.ValueKind == JsonValueKind.Array)
+                VerifyJsonElementIsNotArrayOrObject(ref element);


Should we be creating a JsonValueTrimmable instance in the case where it is a JsonElement instead?

eerhardt · 2021-05-20T16:52:00Z

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonValue.cs

+        /// </returns>
+        /// <typeparam name="T">The type of value to create.</typeparam>
+        /// <param name="value">The value to create.</param>
+        /// <param name="jsonTypeInfo">The <see cref="JsonTypeInfo"/> that is later used to serialize the value.</param>


It might be better to say

The <see cref="JsonTypeInfo"/> that will be used to serialize the value.

Follow up to dotnet#52934. - Using JsonNode in dynamic statements is not trim compatible. Add a LibraryBuild warning since there isn't a direct API to put the warning on. - Mark JsonValueNotTrimmable's ctor as unsafe - Fix up a few warning messages - minor doc fixup Contributes to dotnet#45623

Follow up to #52934. - Using JsonNode in dynamic statements is not trim compatible. Add a LibraryBuild warning since there isn't a direct API to put the warning on. - Mark JsonValueNotTrimmable's ctor as unsafe - Fix up a few warning messages - minor doc fixup Contributes to #45623

JsonNode linker-safe improvements

190acf9

steveharter added the area-System.Text.Json label May 18, 2021

steveharter added this to the 6.0.0 milestone May 18, 2021

steveharter requested review from eerhardt and layomia May 18, 2021 20:16

steveharter self-assigned this May 18, 2021

steveharter requested review from eiriktsarpalis and jozkee as code owners May 18, 2021 20:16

dotnet-issue-labeler bot added the new-api-needs-documentation label May 18, 2021

eerhardt reviewed May 18, 2021

View reviewed changes

layomia reviewed May 18, 2021

View reviewed changes

Fix issue with object converter pulling in all converters

24f0954

steveharter changed the title ~~JsonNode linker-safe improvements~~ JsonNode trimmability improvements May 19, 2021

Remove ILLink.Suppressions.xml

be99010

layomia approved these changes May 20, 2021

View reviewed changes

steveharter merged commit 595c122 into dotnet:main May 20, 2021

steveharter deleted the NodeApiTrimming branch May 20, 2021 16:39

eerhardt reviewed May 20, 2021

View reviewed changes

runfoapp bot mentioned this pull request May 20, 2021

InvokeCodeThatShouldFirEvents_EnsureEventsFired fails on OSX #52710

Closed

eerhardt mentioned this pull request May 24, 2021

Fully annotate JsonNode for trimmability #53184

Merged

ghost locked as resolved and limited conversation to collaborators Jun 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JsonNode trimmability improvements #52934

JsonNode trimmability improvements #52934

steveharter commented May 18, 2021 •

edited

Loading

dotnet-issue-labeler bot commented May 18, 2021

ghost commented May 18, 2021

eerhardt commented May 18, 2021

eerhardt May 18, 2021

steveharter May 19, 2021

eerhardt May 19, 2021 •

edited

Loading

layomia May 18, 2021 •

edited

Loading

steveharter May 19, 2021

eerhardt May 19, 2021

steveharter May 19, 2021

steveharter May 19, 2021

layomia left a comment

layomia May 20, 2021

steveharter May 20, 2021

eerhardt May 20, 2021

eerhardt May 20, 2021

steveharter May 20, 2021

eerhardt May 20, 2021

eerhardt May 20, 2021

eerhardt May 20, 2021

JsonNode trimmability improvements #52934

JsonNode trimmability improvements #52934

Conversation

steveharter commented May 18, 2021 • edited Loading

dotnet-issue-labeler bot commented May 18, 2021

ghost commented May 18, 2021

eerhardt commented May 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt May 19, 2021 • edited Loading

Choose a reason for hiding this comment

layomia May 18, 2021 • edited Loading

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

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

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 commented May 18, 2021 •

edited

Loading

eerhardt May 19, 2021 •

edited

Loading

layomia May 18, 2021 •

edited

Loading