New serializer converter model for objects and collections

[steveharter]

As part of #1562 this PR refactors the converter model for objects and collection.

No public APIs have changed with this PR - changes were made to be internal for now.

Short-term this PR improves perf and maintainability.

Longer-term a subsequent PR will be created for the proposed public API changes which are expected to add capabilities as explained in the above link (mostly supporting code-gen for AOT and adding public APIs for extensibility and new converter capabilities).

Notes:

• Objects and collections are now treated as a converter (deriving from the existing public JsonConverter class). The previous "main loop" and side infrastructure for objects and collections was removed. This greatly simplifies the many supported collections and increases perf since the previous "main loop" logic can be avoided for each property and\or element; collection elements are no longer boxed.
• The recent "reference handling" feature was refactored due to the changes above.
• Nullable support is now in a converter instead of treating in the previous "main loop".
• Polymorphic support is now in one location and does not require creating a temporary JsonPropertyInfo any longer (and associated cache) so it is much faster.
• Skip\drain support for missing properties is now implemented using the reader's Skip functionality instead of the "main loop" eating the various JSON (the implementation is much faster now). The previous code was written before the reader's Skip functionality existed.
• The readahead functionality continues to work as before (existing converters do not need to be aware that the underlying Stream needs more data).
• The KeyValuePair<TKey, TValue> converter now re-enters the serializer for TKey and TValue without breaking "JsonPath" functionality (if any exceptions are thrown).
• Minimal tests were added or changed. No changes are considered breaking. There are some test coverage areas lacking primarily in the area of collection error handling for Stream scenarios which will be addressed in a future PR.

Performance

The perf benefits are significant for large collections (~1.15x-1.5x on deserialize, ~1.5x-2.4x+ on serialize).

There is currently room for improvement after this PR for additional gains in the areas of:

• The new "preserve reference" handling when not used with the async Stream-based APIs.
• Custom converters when not used with the async Stream-based APIs.
• There is one extra boxing\unboxing per (de)serialize when a struct is passed in at the highest level; this will be removed in a subsequent PR.

Examples of large collection (1,024 elements) perf improvement:
DateTime[]

Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
Deserialize before	129.3 us	1.48 us	1.31 us	129.3 us	127.5 us	131.9 us	3.5605	-	-	24.28 KB
After ~1.3x faster	98.97 us	0.409 us	0.363 us	98.98 us	98.17 us	99.61 us	3.9494	-	-	24.63 KB
Serialize Before	145.2 us	1.19 us	1.11 us	145.3 us	143.8 us	147.5 us	13.3721	1.7442	-	85.24 KB
After ~2x faster	72.90 us	1.344 us	1.122 us	72.38 us	71.60 us	75.60 us	9.8129	1.5333	-	61.52 KB

Dictionary<string,string>

Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
Deserialize Before	190.4 us	1.47 us	1.38 us	190.6 us	188.5 us	193.6 us	26.5554	8.3460	-	163.69 KB
After ~1.2x faster	158.8 us	1.27 us	1.13 us	158.8 us	157.3 us	161.3 us	26.5991	8.8664	-	164.05 KB
Serialize Before	109.7 us	0.77 us	0.72 us	109.5 us	108.5 us	111.1 us	3.4904	-	-	23.92 KB
After ~1.5x faster	74.53 us	0.590 us	0.552 us	74.40 us	73.57 us	75.69 us	3.8179	0.2937	-	24.17 KB

List

Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
Deserialize Before	76.40 us	0.392 us	0.366 us	76.37 us	75.53 us	76.87 us	1.2169	-	-	8.25 KB
After ~1.5x faster	50.05 us	0.251 us	0.235 us	49.94 us	49.76 us	50.43 us	1.3922	-	-	8.62 KB
Serialize Before	29.04 us	0.213 us	0.189 us	29.00 us	28.70 us	29.34 us	1.2620	-	-	8.07 KB
After ~2.4x faster	12.17 us	0.205 us	0.191 us	12.15 us	11.97 us	12.55 us	1.3187	-	-	8.34 KB

Below are all current benchmarks with a difference of >=3%. These are not large collections and show that non-collection perf stayed the same or improved for the most part.

summary:
better: 47, geomean: 1.156
worse: 4, geomean: 1.056
total diff: 51

| Slower                                                                           | diff/base | Base Median (ns) | Diff Median (ns) | Modality|
| -------------------------------------------------------------------------------- | ---------:| ----------------:| ----------------:| --------:|
| System.Text.Json.Serialization.Tests.WriteJson<LoginViewModel>.SerializeToStream |      1.07 |           418.57 |           446.10 |         |
| System.Text.Json.Serialization.Tests.WriteJson<MyEventsListerViewModel>.Serializ |      1.06 |        420929.05 |        447447.86 |         |
| System.Text.Json.Serialization.Tests.WriteJson<BinaryData>.SerializeObjectProper |      1.05 |           737.15 |           774.47 |         |
| System.Text.Json.Serialization.Tests.WriteJson<MyEventsListerViewModel>.Serializ |      1.04 |        456026.25 |        476098.30 |         |

| Faster                                                                           | base/diff | Base Median (ns) | Diff Median (ns) | Modality|
| -------------------------------------------------------------------------------- | ---------:| ----------------:| ----------------:| --------:|
| System.Text.Json.Serialization.Tests.WriteJson<HashSet<String>>.SerializeObjectP |      2.21 |         15823.95 |          7154.65 |         |
| System.Text.Json.Serialization.Tests.WriteJson<Dictionary<String, String>>.Seria |      1.90 |         21761.23 |         11465.60 |         |
| System.Text.Json.Serialization.Tests.WriteJson<ArrayList>.SerializeObjectPropert |      1.66 |         18708.34 |         11242.69 |         |
| System.Text.Json.Serialization.Tests.WriteJson<Hashtable>.SerializeObjectPropert |      1.48 |         26174.08 |         17649.10 |         |
| System.Text.Json.Serialization.Tests.WriteJson<ImmutableDictionary<String, Strin |      1.43 |         42776.34 |         29881.02 |         |
| System.Text.Json.Serialization.Tests.WriteJson<ImmutableSortedDictionary<String, |      1.39 |         29465.01 |         21219.80 |         |
| System.Text.Json.Serialization.Tests.WriteJson<Dictionary<String, String>>.Seria |      1.34 |         14567.50 |         10899.91 |         |
| System.Text.Json.Serialization.Tests.WriteJson<Dictionary<String, String>>.Seria |      1.33 |         13976.36 |         10533.66 |         |
| System.Text.Json.Serialization.Tests.WriteJson<Dictionary<String, String>>.Seria |      1.25 |         14096.39 |         11261.12 |         |
| System.Text.Json.Serialization.Tests.WriteJson<HashSet<String>>.SerializeToUtf8B |      1.23 |          8052.41 |          6521.88 |         |
| System.Text.Json.Serialization.Tests.ReadJson<HashSet<String>>.DeserializeFromSt |      1.19 |         16564.63 |         13915.07 |         |
| System.Text.Json.Serialization.Tests.ReadJson<HashSet<String>>.DeserializeFromUt |      1.18 |         16380.29 |         13868.91 |         |
| System.Text.Json.Serialization.Tests.WriteJson<HashSet<String>>.SerializeToStrin |      1.15 |          8007.31 |          6992.08 |         |
| System.Text.Json.Serialization.Tests.WriteJson<HashSet<String>>.SerializeToStrea |      1.14 |          7682.76 |          6715.29 |         |
| System.Text.Json.Serialization.Tests.ReadJson<HashSet<String>>.DeserializeFromSt |      1.14 |         17129.64 |         14987.80 |         |
| System.Text.Json.Serialization.Tests.ReadJson<Dictionary<String, String>>.Deseri |      1.14 |         22875.54 |         20029.75 |         |
| System.Text.Json.Serialization.Tests.ReadJson<Dictionary<String, String>>.Deseri |      1.13 |         23247.09 |         20542.03 |         |
| System.Text.Json.Serialization.Tests.WriteJson<Hashtable>.SerializeToUtf8Bytes   |      1.11 |         18672.59 |         16878.51 |         |
| System.Text.Json.Serialization.Tests.WriteJson<ImmutableSortedDictionary<String, |      1.10 |         22723.33 |         20610.05 |         |
| System.Text.Json.Serialization.Tests.WriteJson<ImmutableSortedDictionary<String, |      1.10 |         21879.21 |         19966.11 |         |
| System.Text.Json.Serialization.Tests.WriteJson<ImmutableSortedDictionary<String, |      1.09 |         21688.75 |         19807.52 |         |
| System.Text.Json.Serialization.Tests.ReadJson<MyEventsListerViewModel>.Deseriali |      1.09 |        325916.80 |        298750.00 |         |
| System.Text.Json.Serialization.Tests.WriteJson<ImmutableDictionary<String, Strin |      1.09 |         31788.14 |         29206.09 |         |
| System.Text.Json.Serialization.Tests.ReadJson<IndexViewModel>.DeserializeFromStr |      1.08 |         30781.52 |         28528.53 |         |
| System.Text.Json.Serialization.Tests.ReadJson<MyEventsListerViewModel>.Deseriali |      1.08 |        402967.31 |        373768.28 |         |
| System.Text.Json.Serialization.Tests.ReadJson<Location>.DeserializeFromString    |      1.08 |          1310.54 |          1215.74 |         |
| System.Text.Json.Serialization.Tests.ReadJson<IndexViewModel>.DeserializeFromUtf |      1.08 |         28978.98 |         26890.61 |         |
| System.Text.Json.Serialization.Tests.ReadJson<Dictionary<String, String>>.Deseri |      1.07 |         23407.78 |         21812.70 |         |
| System.Text.Json.Serialization.Tests.ReadJson<ArrayList>.DeserializeFromUtf8Byte |      1.07 |         48136.14 |         45138.76 |         |
| System.Text.Json.Serialization.Tests.ReadJson<MyEventsListerViewModel>.Deseriali |      1.06 |        335759.77 |        315525.65 |         |
| System.Text.Json.Serialization.Tests.ReadJson<LoginViewModel>.DeserializeFromUtf |      1.06 |           471.02 |           443.09 |         |
| System.Text.Json.Serialization.Tests.WriteJson<Hashtable>.SerializeToStream      |      1.06 |         18199.73 |         17174.52 |         |
| System.Text.Json.Serialization.Tests.WriteJson<Hashtable>.SerializeToString      |      1.06 |         18664.40 |         17622.58 |         |
| System.Text.Json.Serialization.Tests.WriteJson<ArrayList>.SerializeToString      |      1.06 |         11876.28 |         11214.49 |         |
| System.Text.Json.Serialization.Tests.WriteJson<ImmutableDictionary<String, Strin |      1.06 |         30780.59 |         29166.80 |         |
| System.Text.Json.Serialization.Tests.ReadJson<ArrayList>.DeserializeFromStream   |      1.05 |         49586.90 |         47159.45 |         |
| System.Text.Json.Serialization.Tests.WriteJson<ImmutableDictionary<String, Strin |      1.05 |         30558.31 |         29158.70 |         |
| System.Text.Json.Serialization.Tests.WriteJson<IndexViewModel>.SerializeToUtf8By |      1.05 |         19127.25 |         18252.95 |         |
| System.Text.Json.Serialization.Tests.ReadJson<Location>.DeserializeFromUtf8Bytes |      1.04 |          1222.33 |          1171.81 |         |
| System.Text.Json.Serialization.Tests.ReadJson<LoginViewModel>.DeserializeFromStr |      1.04 |           525.13 |           504.04 |         |
| System.Text.Json.Serialization.Tests.ReadJson<Hashtable>.DeserializeFromString   |      1.04 |         65467.23 |         62855.51 |         |
| System.Text.Json.Serialization.Tests.ReadJson<IndexViewModel>.DeserializeFromStr |      1.03 |         30913.28 |         29904.75 |         |
| System.Text.Json.Serialization.Tests.ReadJson<ArrayList>.DeserializeFromString   |      1.03 |         48737.73 |         47179.26 |         |
| System.Text.Json.Serialization.Tests.WriteJson<ArrayList>.SerializeToUtf8Bytes   |      1.03 |         11342.33 |         10993.20 |         |
| System.Text.Json.Serialization.Tests.ReadJson<Hashtable>.DeserializeFromStream   |      1.03 |         64968.44 |         63013.98 |         |
| System.Text.Json.Serialization.Tests.WriteJson<ArrayList>.SerializeToStream      |      1.03 |         11217.19 |         10892.45 |         |
| System.Text.Json.Serialization.Tests.WriteJson<IndexViewModel>.SerializeObjectPr |      1.03 |         20301.90 |         19732.69 |         |

[jozkee]

Will continue reviewing tomorrow.

[jozkee]

Regarding changes related to ReferenceHandling, most of the things that differs from before are the exception messages. Also I think we should discuss what are the concrete guidelines for building the JSON path in Deserialize errors.

[steveharter]

Currently System.Text.Json does not target NetStandard2.1. If we did add that configuration then we could use RuntimeFeature.IsDynamicCodeSupported to determine the strategy, but adding a 2.1 configuration is likely not going to help with 5.0 since Xamarin and other will just target netcoreapp5.0 instead.

[ahsonkhan]

Let's discuss outside this PR. Maybe we can still use IsDynamicSupported for the netcoreapp configs. Since we also target netstandard2.0 in this library, we'll always have to ifdef that anyway.

[ahsonkhan]

Looks like most of the benchmarks had an allocation increase of about ~410 bytes (both for serialize and deserialize). Do we know where that's coming from?

List
Deserialize
Before: 8.25 KB
After: 8.68 KB

Serialize
Before: 23.92 KB
After: 24.23 KB

[jozkee]

This one is weird:

Before, if we had more than one $id property, e.g. {"$id": "1", "$id" :"2", "$values": []}, we would throw The metadata property '$id' must be the first property in the JSON object..

Now, we are throwing Invalid property '$id' found within a JSON object that *must only contain metadata properties* and the nested JSON array to be preserved. which is weird since $id is indeed a metadata property.

Fixing this would imply to extend the logic to choose what to throw depending on GetMetadataPropertyName, similar to ThrowUnexpectedMetadataException on line 253.

[jozkee]

I can't even think of how we can ensure that on a test, both messages contain the literal '$id' and the Json path is $.$id on both.

[steveharter]

Perhaps if we collapse the exception messages we wouldn't have to condition here?

[ahsonkhan]

Highlighting the breaking change here.

Serializing non-supported dictionaries now deterministically throws (regardless of the runtime values). For example object keys in dictionaries is not supported. Previously, if the runtime type happened to be string, the dictionary was serialized. Other types resulted in NSE. Now we throw NSE in all cases.

public static void TestSerializeStringAsObjecDictionaryKeys()
{
    var notSupportedDictionary = new Dictionary<object, object>
    {
        { "foo", "bar" }
    };

    // Before (3.1): Returned a string with value "{"foo":"bar"}"
    // After (5.0): System.NotSupportedException: The type 'System.Collections.Generic.Dictionary`2[System.Object,System.Object]' is not supported.
    JsonSerializer.Serialize(notSupportedDictionary);
}

[iSazonov]

Is this change in 5.0 Preview2?
In PowerShell repo we get an exception for null parameter:

PS > $null | ConvertTo-Json -Compress
ConvertTo-Json: Value cannot be null. (Parameter 'type')
PS > Get-Error

Exception             :
    Type       : System.ArgumentNullException
    Message    : Value cannot be null. (Parameter 'type')
    ParamName  : type
    TargetSite :
        Name          : VerifyValueAndType
        DeclaringType : System.Text.Json.JsonSerializer
        MemberType    : Method
        Module        : System.Text.Json.dll
    StackTrace :
   at System.Text.Json.JsonSerializer.VerifyValueAndType(Object value, Type type)
   at System.Text.Json.JsonSerializer.Serialize(Object value, Type inputType, JsonSerializerOptions options)
   at Microsoft.PowerShell.Commands.JsonObject.ConvertToJson2(Object objectToProcess, ConvertToJsonContext& context) in
 C:\Users\1\Documents\GitHub\iSazonov\PowerShell\src\Microsoft.PowerShell.Commands.Utility\commands\utility\WebCmdlet\J
sonObject.cs:line 538
   at Microsoft.PowerShell.Commands.ConvertToJsonCommand2.EndProcessing() in C:\Users\1\Documents\GitHub\iSazonov\Power
Shell\src\Microsoft.PowerShell.Commands.Utility\commands\utility\WebCmdlet\ConvertToJsonCommand.cs:line 252
   at System.Management.Automation.Cmdlet.DoEndProcessing() in C:\Users\1\Documents\GitHub\iSazonov\PowerShell\src\Syst
em.Management.Automation\engine\cmdlet.cs:line 187
   at System.Management.Automation.CommandProcessorBase.Complete() in C:\Users\1\Documents\GitHub\iSazonov\PowerShell\s
rc\System.Management.Automation\engine\CommandProcessorBase.cs:line 590
    Source     : System.Text.Json
    HResult    : -2147467261
CategoryInfo          : NotSpecified: (:) [ConvertTo-Json], ArgumentNullException
FullyQualifiedErrorId : System.ArgumentNullException,Microsoft.PowerShell.Commands.ConvertToJsonCommand2
InvocationInfo        :
    MyCommand        : ConvertTo-Json
    ScriptLineNumber : 1
    OffsetInLine     : 9
    HistoryId        : 1
    Line             : $null | ConvertTo-Json -Compress
    PositionMessage  : At line:1 char:9
                       + $null | ConvertTo-Json -Compress
                       +         ~~~~~~~~~~~~~~~~~~~~~~~~
    InvocationName   : ConvertTo-Json
    CommandOrigin    : Internal
ScriptStackTrace      : at <ScriptBlock>, <No file>: line 1

I tried to directly reference System.Text.Json 5.0.0-preview.2.20160.6 but get the same exception.
I wonder to see JsonSerializer.VerifyValueAndType() because the PR removed it.

[danmoseley]

@steveharter this is labeled breaking change for reason noted by @ahsonkhan . Can you please open an issue if necessary with https://github.com/dotnet/docs/issues/new?template=dotnet-breaking-change.md

I don't see an existing one

[steveharter]

@danmosemsft

The breaking change that was pointed out by @ahsonkhan no longer occurs. Here's an updated repro:

            var notSupportedDictionary = new Dictionary<object, object> {{ "foo", "bar" }};

            // Before (3.1): Returned a string with value "{"foo":"bar"}"
            // After (5.0): Now also returns the same
            string json = JsonSerializer.Serialize(notSupportedDictionary);
            Debug.Assert(json == "{\"foo\":\"bar\"}");

[danmoseley]

Even better!

[steveharter]

The only potential breaking change that I'm aware of is calling options.GetConverter(type) now returns non-null (previously it was null).

We should doc this in the release notes, so keeping the breaking change tag probably still makes sense.

[layomia]

The breaking change that was pointed out by @ahsonkhan no longer occurs.

FYI this is due to @jozkee's work on supporting non-string dictionary keys: #38056.