Skip to content

Create and add instance reference inside ResolveMetadata*#39330

Merged
jozkee merged 2 commits intodotnet:masterfrom
jozkee:json_37168
Jul 20, 2020
Merged

Create and add instance reference inside ResolveMetadata*#39330
jozkee merged 2 commits intodotnet:masterfrom
jozkee:json_37168

Conversation

@jozkee
Copy link
Member

@jozkee jozkee commented Jul 14, 2020

...in order to remove workaround described in #37168 used to correctly build the JSON path in case of error.

This PR adds a new method CreateInstance to JsonResumableConverter<T> that is used to instantiate the object at the time that ReferenceResolver.AddReference is called.

It also splits ResolveMetadata into two methods: ResolveMetadataForJsonObject (Used on POCOs and dictionaries) and ResolveMetadataForJsonArray (Used for collections such as List<T>).

This makes the code more mantainable, reduces the amount of if checks and allow us to remove the string field ReadStackFrame.MetadataId from the state.

Fixes #37168.

@jozkee jozkee added this to the 5.0.0 milestone Jul 14, 2020
@jozkee jozkee requested review from layomia and steveharter July 14, 2020 23:01
@jozkee jozkee self-assigned this Jul 14, 2020
@jozkee
Copy link
Member Author

jozkee commented Jul 15, 2020

Perf. results for the stream scenarios affected by this change:

summary:
better: 9, geomean: 1.020
worse: 1, geomean: 1.023
total diff: 10

| Slower                                                                           | diff/base | Base Median (ns) | Diff Median (ns) | Modality|
| -------------------------------------------------------------------------------- | ---------:| ----------------:| ----------------:| --------:|
| System.Text.Json.Serialization.Tests.ReadJson<LoginViewModel>.DeserializeFromStr |      1.02 |           683.30 |           699.20 |         |

| Faster                                                                           | base/diff | Base Median (ns) | Diff Median (ns) | Modality|
| -------------------------------------------------------------------------------- | ---------:| ----------------:| ----------------:| -------- |
| System.Text.Json.Serialization.Tests.ReadJson<SimpleStructWithProperties>.Deseri |      1.05 |           532.98 |           505.85 | bimodal |
| System.Text.Json.Serialization.Tests.ReadJson<IndexViewModel>.DeserializeFromStr |      1.02 |         30798.67 |         30088.40 |         |
| System.Text.Json.Serialization.Tests.ReadJson<LargeStructWithProperties>.Deseria |      1.02 |          1533.54 |          1503.43 |         |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableSortedDictionary<String,  |      1.02 |         75880.01 |         74599.11 |         |
| System.Text.Json.Serialization.Tests.ReadJson<Int32>.DeserializeFromStream       |      1.02 |           262.67 |           258.25 |         |
| System.Text.Json.Serialization.Tests.ReadJson<ImmutableDictionary<String, String |      1.01 |         47830.18 |         47156.48 |         |
| System.Text.Json.Serialization.Tests.ReadJson<HashSet<String>>.DeserializeFromSt |      1.01 |         13382.56 |         13210.33 |         |
| System.Text.Json.Serialization.Tests.ReadJson<Dictionary<String, String>>.Deseri |      1.01 |         22656.52 |         22433.16 |         |
| System.Text.Json.Serialization.Tests.ReadJson<BinaryData>.DeserializeFromStream  |      1.01 |           744.31 |           738.44 |         |

jozkee
jozkee commented Jul 17, 2020
View reviewed changes
...es/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleMetadata.cs
ref ReadStack state,
JsonSerializerOptions options)
{
JsonConverter converter = state.Current.JsonClassInfo.PropertyInfoForClassInfo.ConverterBase;
Copy link
Member Author

@jozkee jozkee Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed JsonConverter from the params to use this one instead.

@jozkee jozkee requested a review from layomia July 17, 2020 00:41
layomia
layomia approved these changes Jul 17, 2020
View reviewed changes
....Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs
}

public sealed override void CreateInstance(ref Utf8JsonReader reader, ref ReadStack state, JsonSerializerOptions options)
internal sealed override void CreateInstanceForReferenceResolver(ref Utf8JsonReader reader, ref ReadStack state, JsonSerializerOptions options)
Copy link
Contributor

@layomia layomia Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type is already internal, so this should be left as public - here and elsewhere.

Copy link
Member Author

@jozkee jozkee Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is inherited from JsonConverter which is public, if I make it public here and in the base, then we would be adding new API to the base type.

@jaredpar jaredpar mentioned this pull request Jul 17, 2020
Closed
@jozkee jozkee merged commit d8d4d9e into dotnet:master Jul 20, 2020
@jozkee jozkee deleted the json_37168 branch July 20, 2020 18:39
jozkee added a commit to jozkee/runtime that referenced this pull request Jul 20, 2020
@jozkee
Create and add instance reference inside ResolveMetadata* (dotnet#39330)
1e80034 
* Create and add instance reference on ResolveMetadata

* Address suggestions
@jozkee jozkee mentioned this pull request Jul 20, 2020
Merged
jozkee added a commit that referenced this pull request Jul 21, 2020
@jozkee
Create and add instance reference inside ResolveMetadata* (#39330) (#…
9a0b34d 
…39645)

* Create and add instance reference on ResolveMetadata

* Address suggestions
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
@jozkee @Jacksondr5
Create and add instance reference inside ResolveMetadata* (dotnet#39330)
a1ecbe4 
* Create and add instance reference on ResolveMetadata

* Address suggestions
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@steveharter steveharter Awaiting requested review from steveharter

1 more reviewer

@layomia layomia layomia approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jozkee jozkee

Labels

area-System.Text.Json

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

Avoid remembering last metadata to point to $id when AddReference fails

2 participants

@jozkee @layomia