Add support for preserve references on JSON#655

jozkee · 2019-12-07T02:00:33Z

No description provided.

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

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

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

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

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

src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlingTests.Serialize.cs

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleObject.cs

* Inline ReferenceHandling logic within main methods. * remove stale Handle*Ref methods * Refactor the code for Deserialization * Create methods to reuse code * Try to isolate Preserve logic as much as I can * Replaced Exceptions for ThrowHelper methods * Remove stale condition on $ref * Add AggressiveInlining to HandlePropertyNameDefault * Inline feature code in Serialization

Add Reference EqualityComparer to Serialize dictionary.

jozkee · 2019-12-12T23:48:41Z

Benchmarks results for Serialize:

BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i7-6700 CPU 3.40GHz (Skylake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=5.0.100-alpha1-015718
  [Host]     : .NET Core 5.0.0 (CoreCLR 5.0.19.55901, CoreFX 5.0.19.55607), X64 RyuJIT
  Job-YXBSEL : .NET Core 5.0.0 (CoreCLR 5.0.19.56303, CoreFX 5.0.19.60601), X64 RyuJIT

Toolchain=CoreRun

Master

Type	Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
WriteJson<ArrayList>	SerializeToUtf8Bytes	10,500.2 ns	174.68 ns	163.40 ns	10,502.4 ns	10,224.4 ns	10,719.6 ns	0.1831	-	-	800 B
WriteJson<BinaryData>	SerializeToUtf8Bytes	506.4 ns	9.86 ns	10.13 ns	507.8 ns	481.5 ns	521.1 ns	0.3729	-	-	1560 B
WriteJson<Dictionary<String, String>>	SerializeToUtf8Bytes	12,591.6 ns	148.42 ns	138.84 ns	12,632.0 ns	12,285.5 ns	12,755.6 ns	0.3204	-	-	1376 B
WriteJson<HashSet<String>>	SerializeToUtf8Bytes	7,301.1 ns	134.14 ns	125.48 ns	7,306.5 ns	7,069.3 ns	7,472.1 ns	0.1831	-	-	792 B
WriteJson<Hashtable>	SerializeToUtf8Bytes	17,697.4 ns	326.87 ns	272.95 ns	17,684.8 ns	17,239.8 ns	18,129.5 ns	0.3052	-	-	1376 B
WriteJson<ImmutableDictionary<String, String>>	SerializeToUtf8Bytes	31,238.8 ns	280.66 ns	262.53 ns	31,268.3 ns	30,430.3 ns	31,500.2 ns	0.3662	-	-	1624 B
WriteJson<ImmutableSortedDictionary<String, String>>	SerializeToUtf8Bytes	21,624.6 ns	353.50 ns	313.37 ns	21,727.5 ns	20,939.4 ns	22,153.7 ns	0.3357	-	-	1520 B
WriteJson<IndexViewModel>	SerializeToUtf8Bytes	22,789.0 ns	277.80 ns	246.26 ns	22,726.4 ns	22,421.3 ns	23,196.9 ns	3.1128	-	-	13056 B
WriteJson<Location>	SerializeToUtf8Bytes	1,082.2 ns	9.98 ns	8.85 ns	1,081.2 ns	1,066.5 ns	1,101.2 ns	0.0896	-	-	384 B
WriteJson<LoginViewModel>	SerializeToUtf8Bytes	437.7 ns	10.80 ns	31.34 ns	422.0 ns	406.2 ns	524.4 ns	0.0625	-	-	264 B
WriteJson<MyEventsListerViewModel>	SerializeToUtf8Bytes	529,185.5 ns	8,012.31 ns	7,494.72 ns	528,999.6 ns	514,734.3 ns	539,885.2 ns	47.8516	8.7891	-	205041 B

Feature

Type	Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
WriteJson<ArrayList>	SerializeToUtf8Bytes	10,374.0 ns	203.15 ns	190.03 ns	10,350.4 ns	10,031.4 ns	10,684.3 ns	0.1831	-	-	800 B
WriteJson<BinaryData>	SerializeToUtf8Bytes	507.4 ns	10.01 ns	10.71 ns	506.2 ns	488.6 ns	531.6 ns	0.3729	-	-	1560 B
WriteJson<Dictionary<String, String>>	SerializeToUtf8Bytes	12,187.8 ns	185.36 ns	173.39 ns	12,214.8 ns	11,911.3 ns	12,468.0 ns	0.3204	-	-	1376 B
WriteJson<HashSet<String>>	SerializeToUtf8Bytes	7,702.8 ns	138.40 ns	122.69 ns	7,743.9 ns	7,398.9 ns	7,880.9 ns	0.1831	-	-	792 B
WriteJson<Hashtable>	SerializeToUtf8Bytes	17,809.9 ns	316.10 ns	295.68 ns	17,829.3 ns	17,183.1 ns	18,286.2 ns	0.3052	-	-	1376 B
WriteJson<ImmutableDictionary<String, String>>	SerializeToUtf8Bytes	32,820.3 ns	347.04 ns	324.62 ns	32,901.4 ns	32,342.9 ns	33,311.4 ns	0.3662	-	-	1624 B
WriteJson<ImmutableSortedDictionary<String, String>>	SerializeToUtf8Bytes	21,566.0 ns	403.83 ns	604.43 ns	21,487.7 ns	20,617.3 ns	23,049.1 ns	0.3357	-	-	1520 B
WriteJson<IndexViewModel>	SerializeToUtf8Bytes	23,110.7 ns	222.02 ns	196.82 ns	23,087.3 ns	22,778.7 ns	23,515.5 ns	3.1128	-	-	13056 B
WriteJson<Location>	SerializeToUtf8Bytes	1,070.0 ns	20.30 ns	20.85 ns	1,072.1 ns	1,021.9 ns	1,107.7 ns	0.0916	-	-	384 B
WriteJson<LoginViewModel>	SerializeToUtf8Bytes	416.4 ns	3.22 ns	2.69 ns	416.2 ns	411.9 ns	422.0 ns	0.0625	-	-	264 B
WriteJson<MyEventsListerViewModel>	SerializeToUtf8Bytes	527,382.2 ns	9,137.83 ns	8,547.53 ns	529,095.7 ns	508,687.4 ns	537,437.5 ns	47.8516	8.7891	-	204800 B

jozkee · 2019-12-12T23:51:38Z

Benchmark results for Deserialize:

Master

Type	Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
ReadJson<ArrayList>	DeserializeFromUtf8Bytes	55,070.3 ns	894.47 ns	746.92 ns	55,241.0 ns	52,985.2 ns	55,981.3 ns	6.8359	0.0610	-	28592 B
ReadJson<BinaryData>	DeserializeFromUtf8Bytes	591.0 ns	10.08 ns	9.43 ns	588.0 ns	579.8 ns	607.4 ns	0.2556	-	-	1072 B
ReadJson<Dictionary<String, String>>	DeserializeFromUtf8Bytes	21,088.6 ns	392.14 ns	402.70 ns	21,132.1 ns	20,334.3 ns	21,867.0 ns	3.8757	0.0610	-	16304 B
ReadJson<HashSet<String>>	DeserializeFromUtf8Bytes	16,632.2 ns	308.43 ns	273.42 ns	16,612.7 ns	16,250.4 ns	17,246.9 ns	2.5024	-	-	10496 B
ReadJson<Hashtable>	DeserializeFromUtf8Bytes	68,753.1 ns	984.69 ns	921.08 ns	68,582.4 ns	67,409.2 ns	70,215.7 ns	9.1553	-	-	38344 B
ReadJson<ImmutableDictionary<String, String>>	DeserializeFromUtf8Bytes	61,368.9 ns	1,377.77 ns	1,288.76 ns	61,170.4 ns	59,817.1 ns	64,360.5 ns	6.2256	0.4883	-	26219 B
ReadJson<ImmutableSortedDictionary<String, String>>	DeserializeFromUtf8Bytes	144,746.6 ns	1,878.24 ns	1,665.01 ns	144,821.5 ns	142,494.2 ns	148,153.7 ns	7.8125	-	-	33068 B
ReadJson<IndexViewModel>	DeserializeFromUtf8Bytes	34,959.6 ns	675.49 ns	693.68 ns	34,721.2 ns	34,097.0 ns	36,367.8 ns	5.3711	-	-	22472 B
ReadJson<Location>	DeserializeFromUtf8Bytes	1,375.4 ns	19.20 ns	17.96 ns	1,373.1 ns	1,344.4 ns	1,405.5 ns	0.1049	-	-	448 B
ReadJson<LoginViewModel>	DeserializeFromUtf8Bytes	526.3 ns	9.60 ns	8.02 ns	521.9 ns	517.6 ns	545.2 ns	0.0401	-	-	168 B
ReadJson<MyEventsListerViewModel>	DeserializeFromUtf8Bytes	397,268.2 ns	3,801.78 ns	3,556.18 ns	397,510.9 ns	390,277.7 ns	402,942.6 ns	18.5547	0.9766	-	78680 B

Feature

Type	Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
ReadJson<ArrayList>	DeserializeFromUtf8Bytes	54,932.4 ns	1,014.06 ns	948.55 ns	55,131.0 ns	52,991.3 ns	56,305.6 ns	6.8359	0.0610	-	28592 B
ReadJson<BinaryData>	DeserializeFromUtf8Bytes	629.8 ns	10.25 ns	9.59 ns	628.2 ns	613.1 ns	643.5 ns	0.2556	-	-	1072 B
ReadJson<Dictionary<String, String>>	DeserializeFromUtf8Bytes	21,805.0 ns	581.31 ns	485.42 ns	21,639.8 ns	21,434.8 ns	23,230.8 ns	3.8757	0.1221	-	16304 B
ReadJson<HashSet<String>>	DeserializeFromUtf8Bytes	17,077.0 ns	182.16 ns	170.40 ns	17,112.8 ns	16,762.6 ns	17,363.0 ns	2.5024	-	-	10496 B
ReadJson<Hashtable>	DeserializeFromUtf8Bytes	69,689.8 ns	1,046.58 ns	978.98 ns	70,064.1 ns	67,680.1 ns	71,107.5 ns	9.1553	-	-	38344 B
ReadJson<ImmutableDictionary<String, String>>	DeserializeFromUtf8Bytes	62,285.9 ns	1,241.23 ns	1,219.06 ns	62,499.0 ns	60,032.3 ns	63,793.3 ns	6.2256	-	-	26219 B
ReadJson<ImmutableSortedDictionary<String, String>>	DeserializeFromUtf8Bytes	142,799.4 ns	2,217.03 ns	2,073.81 ns	142,754.6 ns	140,153.9 ns	146,993.1 ns	7.8125	0.4883	-	33068 B
ReadJson<IndexViewModel>	DeserializeFromUtf8Bytes	36,049.0 ns	678.49 ns	634.66 ns	36,294.2 ns	34,789.0 ns	36,964.4 ns	5.3711	-	-	22537 B
ReadJson<Location>	DeserializeFromUtf8Bytes	1,460.7 ns	14.98 ns	14.01 ns	1,459.7 ns	1,435.2 ns	1,490.9 ns	0.1068	-	-	448 B
ReadJson<LoginViewModel>	DeserializeFromUtf8Bytes	537.2 ns	10.36 ns	10.64 ns	540.1 ns	515.9 ns	550.6 ns	0.0401	-	-	168 B
ReadJson<MyEventsListerViewModel>	DeserializeFromUtf8Bytes	398,516.0 ns	5,697.81 ns	5,329.74 ns	400,372.0 ns	387,206.7 ns	405,554.3 ns	18.5547	5.3711	-	78745 B

…issues.

… when enumerable is already initialized.

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

.../System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleDictionary.cs

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleObject.cs

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

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

src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlingTests.Deserialize.cs

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

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

* Switched ReferenceResolver to access it through a read-only property that initializes it the first time is tried to be accesed. * Renamed some methods and properties * Refactored WriteReference* methods. * Fixed Exception messages * Removed literal message comparison on tests.

* Fix issues found with Round-tripping scenarios * Move DictionaryPropertyIsPreserved to EndProperty() in order to fix issue where two dictionary properties were next to each other and the DictionaryPropertyIsPreserved was not reset. * Add ReadStackFrame.IsNestedPreservedArray in order to identify preserved arrays nested and prevent using setPropertyDirectly on them.

…enceHandlingAndNullability

…sted in a Dictionary.

jozkee · 2020-01-11T03:08:55Z

There are a few non-trivial thighs left to do:

Add logic to resolve a preserved reference instead of using a converter in case there is any.
Check if its doable to remove JsonPreservedReference<T>.
Hook up JsonPropertyInfoAsString to avoid using global s_metadataProperty (this can be done on the new converter model refactor).

Can we move those for future (in a new issue) so we can merge?

@ahsonkhan, @layomia, @steveharter

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

* Change ReferenceResolver from class to struct to avoid one allocation. * Replace propertyName.ToArray() for constant arrays in order to avoid allocation.

steveharter

All functional concerns addressed. Thanks for all the research and work on this very important feature!

* Use PropertyCacheArray on Values property lookup. * Make Comparer static field to avoid one allocation at runtime. * Document reasoning for DefaultReferenceResolver and ReferenreEqualsEqualityComparer

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

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

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

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

src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj

* Add documentation to DefaultReferenceResolver public methods.

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

...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonPreservableArrayReference.cs

...es/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleMetadata.cs

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

src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlingTests.Deserialize.cs

ahsonkhan · 2020-01-18T11:28:16Z

src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlingTests.Serialize.cs

+                //Struct as array element (same as struct being root).
+                ImmutableArray.Create(employee);
+
+            // Regardless of using preserve, do not emit $id to value types; that is why we compare against default.


This is a helpful comment.

ahsonkhan · 2020-01-18T13:48:26Z

src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlingTests.Deserialize.cs

+        }
+
+        //NOTE: If you implement a converter, you are on your own when handling metadata properties and therefore references.Newtonsoft does the same.
+        //However; is there a way to recall preserved references previously found in the payload and to store new ones found in the converter's payload? that would be a cool enhancement.


It sure would be a cool enhancement :)

* On $values, set PropertyName on state.Current.JsonPropertyName instead.

ahsonkhan

Looks great. Thanks! Can't wait for folks to try out the feature and give feedback.

ghost · 2020-01-18T14:24:04Z

Hello @jozkee!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ahsonkhan · 2020-01-18T14:29:12Z

Here are the list of issues to address (in subsequent PRs). Thanks for filing them to keep track, @jozkee:
#1776
#1780
#1783
#1881
#1882
#1902
#1903

psmulovics · 2020-05-20T02:18:58Z

Someone should update https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-migrate-from-newtonsoft-how-to#preserve-object-references-and-handle-loops accordingly

danmoseley · 2020-05-20T03:55:24Z

@psmulovics there's an edit button in the top right if you're offering 😺

jozkee added 3 commits December 6, 2019 16:55

Initial commit for JSON reference handling / preserve references

d3848c5

Remove ReferenceHandling.Ignore option

1007de8

Code clean-up.

996e401

jozkee added the area-System.Text.Json label Dec 7, 2019

jozkee requested review from ahsonkhan, layomia and steveharter December 7, 2019 02:00

Remove stale tests.

b1c684d

This comment has been minimized.

Sign in to view

layomia reviewed Dec 9, 2019

View reviewed changes

jozkee commented Dec 10, 2019

View reviewed changes

...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleObject.cs Outdated Show resolved Hide resolved

jozkee added 4 commits December 10, 2019 16:10

Address PR feedback

f416d42

Fix preserve references for ExtensionData

3774c1b

Split Reference dictionary into two, for (De)Serialize each.

b5aaca6

Add Reference EqualityComparer to Serialize dictionary.

jozkee requested a review from layomia December 12, 2019 23:00

jozkee added 5 commits December 12, 2019 17:26

Do not set PropertyName to s_missingProperty to avoid race condition …

46ded84

…issues.

Remove Asserts that compare against an exception message.

5628604

Set preserved array passing setPropertyDirectly = true to avoid issue…

96a019b

… when enumerable is already initialized.

Code clean-up.

56748f9

Separate write code into WritePreservedObject and WriteReferenceObject

561ee64

layomia reviewed Dec 14, 2019

View reviewed changes

ahsonkhan mentioned this pull request Jan 1, 2020

New article - Migrate from Newtonsoft.Json dotnet/docs#16225

Merged

danmoseley reviewed Jan 7, 2020

View reviewed changes

src/libraries/System.Text.Json/src/Resources/Strings.resx Outdated Show resolved Hide resolved

danmoseley reviewed Jan 7, 2020

View reviewed changes

src/libraries/System.Text.Json/src/Resources/Strings.resx Show resolved Hide resolved

jozkee added 3 commits January 7, 2020 13:38

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

af7d8c6

…enceHandlingAndNullability

jozkee added 2 commits January 10, 2020 20:33

Consolidated MetadataPropertyName enum logic on read and write.

c4913a8

Reuse HandleStartObjectInEnumerable to handle arrays with metadata ne…

8102079

…sted in a Dictionary.

steveharter reviewed Jan 13, 2020

View reviewed changes

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

Refactor code:

c9d0f2d

* Change ReferenceResolver from class to struct to avoid one allocation. * Replace propertyName.ToArray() for constant arrays in order to avoid allocation.

steveharter approved these changes Jan 14, 2020

View reviewed changes

Move some exception logic to the ThrowHelper class.

4680580

jozkee force-pushed the ReferenceHandling branch from 7bcf47b to c8a4ed5 Compare January 15, 2020 03:49

* Apply optimizations:

c0befe5

* Use PropertyCacheArray on Values property lookup. * Make Comparer static field to avoid one allocation at runtime. * Document reasoning for DefaultReferenceResolver and ReferenreEqualsEqualityComparer

jozkee force-pushed the ReferenceHandling branch from c8a4ed5 to c0befe5 Compare January 15, 2020 03:54

ahsonkhan reviewed Jan 16, 2020

View reviewed changes

jozkee added 2 commits January 17, 2020 02:06

Address PR comments.

53b12ad

Addres more PR feedback:

2b05e02

* Add documentation to DefaultReferenceResolver public methods.

ahsonkhan reviewed Jan 17, 2020

View reviewed changes

jozkee added 2 commits January 17, 2020 22:41

Address PR feedback.

da9cf04

Adderss more PR suggestions.

2174770

ahsonkhan reviewed Jan 18, 2020

View reviewed changes

* Move tests to Serialization namespace.

a0db554

* On $values, set PropertyName on state.Current.JsonPropertyName instead.

ahsonkhan approved these changes Jan 18, 2020

View reviewed changes

jozkee added the auto-merge label Jan 18, 2020

jozkee merged commit 339b447 into dotnet:master Jan 19, 2020

layomia mentioned this pull request Apr 28, 2020

JsonResult with Newtonsoft.Json results in synchronous IO in large responses dotnet/aspnetcore#21187

Closed

layomia added this to the 5.0 milestone May 15, 2020

layomia assigned jozkee May 15, 2020

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

jozkee deleted the ReferenceHandling branch March 24, 2021 19:36

Conversation

jozkee commented Dec 7, 2019

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jozkee commented Dec 12, 2019

Uh oh!

jozkee commented Dec 12, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jozkee commented Jan 11, 2020

Uh oh!

Uh oh!

steveharter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahsonkhan Jan 18, 2020

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Jan 18, 2020

Choose a reason for hiding this comment

Uh oh!

ahsonkhan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 18, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

ahsonkhan commented Jan 18, 2020

Uh oh!

psmulovics commented May 20, 2020

Uh oh!

danmoseley commented May 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

ahsonkhan left a comment •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.