Add support for multiple non-string TKey on dictionaries #38056

Jozkee · 2020-06-17T22:51:09Z

Second attempt to fix #30524 (previos attempt: #32909)

Enables support for multiple non-string Dictionary key types.

For object keys the following applies:

Enables support on serialization if the runtime type is one of the supported types.
They remain unsupported on deserialization.

The DictionaryKeyPolicy defined on JsonSerializerOptions will only apply for keys of type string.

This PR also defers unsupported key validation i.e. we will throw NSE until we use the Key Converter, meaning that empty or null dictionaries won't throw even when their key is not supported.

layomia · 2020-06-18T17:50:09Z

Looks like we haven't added parsing/writing logic for every type listed here: https://github.com/dotnet/runtime/pull/32676/files#diff-899ea4f41d4cbaeaa272936e99afcf9aR41-R58.

Jozkee · 2020-06-18T17:59:29Z

I wanted to hold-off extending ReadWithQuotes/WriteWithQuotes to the other types until we confirmed that this was the correct approach. The implementation for the types left shouldn't be more than copy/paste the code from Int32Converter.

...tem.Text.Json/tests/Serialization/CollectionTests/CollectionTests.Dictionary.NonStringKey.cs

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs

....Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs

layomia · 2020-06-18T17:57:50Z

....Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs

-
-                        state.Current.JsonPropertyNameAsString = reader.GetString();
+                        ReadOnlySpan<byte> propertyName = JsonSerializer.GetPropertyName(ref state, ref reader, options);
+                        state.Current.DictionaryKeyName = propertyName.ToArray();


So this is only used for continuation cases...

...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/EnumConverter.cs

...ries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/StringConverter.cs

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs

…support

src/libraries/System.Text.Json/src/Resources/Strings.resx

...aries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/Int16Converter.cs

...ries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/StringConverter.cs

...m.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryConverter.cs

....Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs

...raries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/GuidConverter.cs

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs

...braries/System.Text.Json/tests/Serialization/CollectionTests/CollectionTests.Generic.Read.cs

...tem.Text.Json/tests/Serialization/CollectionTests/CollectionTests.Dictionary.NonStringKey.cs

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

…rite 2. Address some feedback

layomia · 2020-07-02T18:01:37Z

EDIT: There is a small regression on Utf8JsonReader.Get* methods caused by the addition of the helper *Core methods. This can be potentially mitigated by removing the helpers and just duplicate the code.

If this is still the case, let's duplicate the code for the worst regressions i.e. >= 4%.

Jozkee · 2020-07-04T00:46:27Z

That's no longer the case, I ran the benchmarks again and the results with TryGet*Core are more or less the same, plus I also verified that the produced assembly was getting the TryGet*Core methods inlined on the existing ones.

summary:
worse: 10, geomean: 1.013
total diff: 10

| Slower                                            | diff/base | Base Median (ns) | Diff Median (ns) | Modality|
| ------------------------------------------------- | ---------:| ----------------:| ----------------:| --------:|
| System.Text.Json.Tests.Perf_Get.GetInt32          |      1.04 |           774.68 |           801.96 |         |
| System.Text.Json.Tests.Perf_Get.GetSingle         |      1.02 |          3961.69 |          4053.06 |         |
| System.Text.Json.Tests.Perf_Get.GetString         |      1.02 |          3118.40 |          3173.07 |         |
| System.Text.Json.Tests.Perf_Get.GetDateTime       |      1.01 |          3614.62 |          3666.47 |         |
| System.Text.Json.Tests.Perf_Get.GetUInt32         |      1.01 |           727.07 |           735.23 |         |
| System.Text.Json.Tests.Perf_Get.GetSByte          |      1.01 |          1436.06 |          1447.93 |         |
| System.Text.Json.Tests.Perf_Get.GetDateTimeOffset |      1.01 |          5451.09 |          5491.71 |         |
| System.Text.Json.Tests.Perf_Get.GetInt64          |      1.01 |           847.79 |           854.10 |         |
| System.Text.Json.Tests.Perf_Get.GetGuid           |      1.01 |          5292.47 |          5324.14 |         |
| System.Text.Json.Tests.Perf_Get.GetDecimal        |      1.00 |          4744.43 |          4759.51 |         |

No Faster results for the provided threshold = 0% and noise filter = 0.3ns.

layomia

LGTM, thank you!

layomia · 2020-07-06T06:56:20Z

.../src/System/Text/Json/Serialization/Converters/Collection/DictionaryOfTKeyTValueConverter.cs

    {
-        protected override void Add(in TValue value, JsonSerializerOptions options, ref ReadStack state)
+        protected override void Add(TKey key, in TValue value, JsonSerializerOptions options, ref ReadStack state)


Any measurable perf benefit for non-string keys if we make this in TKey key?

I don't think it is a good idea to use in given that none of the supported types would be a readonly struct or a large struct. Theoretically there should not be benefit from doing so, on the contrary, the perf. would probably decrease given the fact that a defensive copy needs to be created every time Add(in TKey key, ) is called.

cc @GrabYourPitchforks

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs

layomia · 2020-07-06T07:01:25Z

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs

+            JsonConverter GetEnumConverter()
+                => (JsonConverter)Activator.CreateInstance(
+                        typeof(EnumConverter<>).MakeGenericType(keyType),
+                        BindingFlags.Instance | BindingFlags.Public,
+                        binder: null,
+                        new object[] { EnumConverterOptions.AllowStrings | EnumConverterOptions.AllowNumbers, this },
+                        culture: null)!;


Should this code live in the EnumConverterFactory?

I think is better to have it here since it is only useful in this method.
I can add a comment explaining that this is performed to support multiple enum types as dictionary key and we want to use our own defined semantics (we don't want to call a custom converter).

...tem.Text.Json/tests/Serialization/CollectionTests/CollectionTests.Dictionary.NonStringKey.cs

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs

steveharter · 2020-07-06T16:17:12Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

@@ -433,5 +431,14 @@ internal void VerifyWrite(int originalDepth, Utf8JsonWriter writer)
        /// <param name="value">The value to convert.</param>
        /// <param name="options">The <see cref="JsonSerializerOptions"/> being used.</param>
        public abstract void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options);
+
+        internal virtual T ReadWithQuotes(ref Utf8JsonReader reader)


I believe we previously discussed this, but if someone creates a custom converter such as for Int32 then they will get the default semantics when using quoted keys, not the custom semantics, correct?

If\when we want to support custom converters for quoted keys, then we can add support for exposing ReadWithQuotes publicly or find some other way to do this.

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs

steveharter · 2020-07-06T16:31:26Z

...m.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IDictionaryConverter.cs

        where TCollection : IDictionary
    {
-        protected override void Add(in object? value, JsonSerializerOptions options, ref ReadStack state)
+        protected override void Add(string key, in object? value, JsonSerializerOptions options, ref ReadStack state)


I assume IDictionary (non-generic) derived classes can have non-string keys? Do we call ToString() on the underlying object? Do we have tests for this?

IDictionary does not support desereialization of non-string keys given that we don't have a concrete type to parse to, we only use string keys in this case.

steveharter · 2020-07-06T16:32:50Z

...src/System/Text/Json/Serialization/Converters/Collection/IDictionaryOfTKeyTValueConverter.cs

-        : DictionaryDefaultConverter<TCollection, TValue>
-        where TCollection : IDictionary<string, TValue>
+    internal sealed class IDictionaryOfTKeyTValueConverter<TCollection, TKey, TValue>
+        : DictionaryDefaultConverter<TCollection, TKey, TValue>


Nice job that we can use use a single class for this.

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs

layomia · 2020-07-06T23:10:59Z

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs

+        private static ConcurrentDictionary<Type, JsonConverter> GetDictionaryKeyConverters()
+        {
+            const int NumberOfConverters = 18;
+            var converters = new ConcurrentDictionary<Type, JsonConverter>(Environment.ProcessorCount, NumberOfConverters);


nit: if we keep this cache, use the parameterless ConcurrentDictionary ctor here? See #36726 (comment) for a related discussion.

The parameterless ctor internally uses the following.

private static int DefaultConcurrencyLevel => Environment.ProcessorCount;

This would achieve the same with the bonus of allowing us define a initial capacity to avoid resizing over and over.
The difference with #36726 (comment) is that here, the capacity will only grow in the Enum case, which I don't think is very common.

Add support for multiple non-string TKey on dictionaries

f222f23

Jozkee added the area-System.Text.Json label Jun 17, 2020

Jozkee requested review from layomia and steveharter as code owners June 17, 2020 22:51

Jozkee self-assigned this Jun 17, 2020

Fix build errors

25e654b

Jozkee added this to In progress in System.Text.Json - 6.0 Jun 18, 2020

This comment has been minimized.

Sign in to view

layomia reviewed Jun 18, 2020

View reviewed changes

Jozkee added 6 commits June 18, 2020 22:58

Fix performance regression

77b7a4a

Merge branch 'master' of https://github.com/dotnet/runtime into tkey_…

225f27a

…support

Add Read/WriteWithQuotes to the remaining supported types

6bcb25e

Add support for a few more missing types

c272684

Add policy support for Enum keys

eb75a96

Address performance regression.

8d4fd95

This comment has been minimized.

Sign in to view

Fix test error on netstandard

f9b507c

Jozkee mentioned this pull request Jun 25, 2020

Add specification for extending supported types of TKey in ditionaries. #32676

Merged

Merge branch 'master' of https://github.com/dotnet/runtime into tkey_…

3f2a4e7

…support

Jozkee mentioned this pull request Jun 26, 2020

Add benchmarks for Utf8JsonReader.Get* dotnet/performance#1381

Merged

This comment has been minimized.

Sign in to view

layomia reviewed Jun 29, 2020

View reviewed changes

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs Show resolved Hide resolved

Jozkee added 3 commits June 30, 2020 03:09

1. Add support for escaped characters on Read and avoid escaping on W…

578138e

…rite 2. Address some feedback

Remove TryGet*Core methods to mitigate perf regression

2d56bfb

Remove duplicated escaping tests

637c68f

Jozkee requested a review from layomia June 30, 2020 23:24

Jozkee added 4 commits July 1, 2020 18:57

Cache TKey and TValue converters

7201ccb

Remove duplicated code used to read the dictionary key

2b30299

Bring back TryGet*Core

ee17991

Fix test errors with floating point types

8cd787b

layomia approved these changes Jul 6, 2020

View reviewed changes

steveharter reviewed Jul 6, 2020

View reviewed changes

Address nits and feedback from steveharter

8b30e75

Jozkee mentioned this pull request Jul 6, 2020

Test failure: System.IO.Tests.FileSystemWatcherTests_netstandard17/EndInit_ResumesPausedEnableRaisingEvents(setBeforeBeginInit: True) #24181

Closed

layomia reviewed Jul 6, 2020

View reviewed changes

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs Show resolved Hide resolved

layomia reviewed Jul 6, 2020

View reviewed changes

Jozkee merged commit 4d579de into dotnet:master Jul 7, 2020

System.Text.Json - 6.0 automation moved this from In progress to Done Jul 7, 2020

Jozkee deleted the tkey_support branch July 8, 2020 00:59

layomia mentioned this pull request Aug 4, 2020

New serializer converter model for objects and collections #2259

Merged

layomia mentioned this pull request Aug 26, 2020

System.Text.Json does not support TypeConverters #38812

Closed

layomia mentioned this pull request Nov 25, 2020

System.Text.Json doesn't deserialize IDictionary with TKey types other than string #44726

Closed

dotnet locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple non-string TKey on dictionaries #38056

Add support for multiple non-string TKey on dictionaries #38056

Jozkee commented Jun 17, 2020 •

edited

This comment has been minimized.

layomia commented Jun 18, 2020

Jozkee commented Jun 18, 2020

layomia Jun 18, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

layomia commented Jul 2, 2020

Jozkee commented Jul 4, 2020 •

edited

layomia left a comment

layomia Jul 6, 2020

Jozkee Jul 6, 2020 •

edited

layomia Jul 6, 2020

Jozkee Jul 6, 2020

steveharter Jul 6, 2020

steveharter Jul 6, 2020

Jozkee Jul 6, 2020

steveharter Jul 6, 2020

layomia Jul 6, 2020

Jozkee Jul 6, 2020

Add support for multiple non-string TKey on dictionaries #38056

Add support for multiple non-string TKey on dictionaries #38056

Conversation

Jozkee commented Jun 17, 2020 • edited

This comment has been minimized.

layomia commented Jun 18, 2020

Jozkee commented Jun 18, 2020

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

layomia commented Jul 2, 2020

Jozkee commented Jul 4, 2020 • edited

layomia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jozkee Jul 6, 2020 • edited

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

Jozkee commented Jun 17, 2020 •

edited

Jozkee commented Jul 4, 2020 •

edited

Jozkee Jul 6, 2020 •

edited