Expose ReferenceResolver and rename ReferenceHandling to ReferenceHandler #36829

jozkee · 2020-05-21T19:02:58Z

Address the following as it was detailed in #30820 (comment):

The naming changes for ReferenceHandling to resemble a class.
Exposure of the ReferenceResolver class.
Add ReferenceHandler<T> where T : ReferenceResolver to allow users to safely provide their own resolver implementation that will be instantiated on each call to (De)Serialize.
Make ReferenceHandler abstract so if you override it you can opt-in for using a persistent resolver.

This PR effectively closes #30820 as no more changes would be pending to address.

Dotnet-GitSync-Bot · 2020-05-21T19:03:02Z

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.

jozkee · 2020-05-21T21:22:47Z

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

Is it OK to use null as the "default" or as the "do not preserve" option?
As per early discussions with @ahsonkhan, people tend to prefer actually seeing the default option listed, so this would only mask null value as the default, instead of having one instance that holds a ReferenceResolver that is never used.

If Default is not functional, can we consider removing it from the API? The Default property on JsonNamingPolicy is internal.

layomia

Does the change regress existing benchmarks (both with and without the feature)?

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

layomia · 2020-05-21T21:02:51Z

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

Can you minimize the diff in this file by restricting changes just to the new API? We can follow up with a clean-up PR.

layomia · 2020-05-21T21:03:48Z

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

Why is this nullable?

Because it holds a null value on the implementation. As we discussed in another comment, this property can be just a facade for the default value for ReferenceHandler.

I removed the property since as you mentioned in another comment, it is no longer functional, this still might need approval since we are changing the API.

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

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

layomia · 2020-05-21T22:52:39Z

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

What's the limitation with setting the JSON path correctly?

Before, AddReferenceOnDeserialize behave as a TryAdd method, where you could know if the new object was successfully added or not afterwards and we could throw in the call site instead of throwing in the method.

Now, AddReference is a void method that does not follow a TryAdd pattern so we should throw in its implementation, where the code is unaware of the state.

There are a few workaround to keep the behavior as it was before, e.g, we could set state.Current.JsonPropertyName = "$id" before calling AddReference and set it back to its original value after, but that seems quite ugly and no worth doing without discussion. That's why I left this comment here.

Why would you have to set it back to the original value? Current is for the specific frame, and we are doing a forward-only read.

As we discussed offline, there is one case where we are off of $id and we call AddReference: for a preserved array the last metadata we will read before calling AddReference would be $values, i.e. the reader is at the StartArray token when we call AddReference on the IEnumerableDefaultConverter.

As you suggested, we could potentially avoid the "hacky" thing of remember the last metadata if we call AddReference right when we read the $id property instead of waiting for $values. This probably requires splitting HandleMetadata implementation to instantiate the collection when we call HandleMetadata from IEnumerableDefaultconverter.

I have created issue #37168 to follow up on this.

src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.cs

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

src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.cs

…dler

jozkee · 2020-05-29T18:13:00Z

This change does not regress performance on the out-of-box path.

Read

results

Before:

Type	Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
ReadJson<ArrayList>	DeserializeFromUtf8Bytes	50,628.0 ns	238.56 ns	199.21 ns	50,577.9 ns	50,413.1 ns	51,149.3 ns	4.8761	0.6095	-	31016 B
ReadJson<BinaryData>	DeserializeFromUtf8Bytes	522.8 ns	4.57 ns	4.28 ns	522.0 ns	514.8 ns	530.4 ns	0.1689	-	-	1072 B
ReadJson<Dictionary<String, String>>	DeserializeFromUtf8Bytes	20,494.8 ns	123.63 ns	115.65 ns	20,442.9 ns	20,364.6 ns	20,738.1 ns	4.0690	0.5697	-	25872 B
ReadJson<HashSet<String>>	DeserializeFromUtf8Bytes	13,761.3 ns	125.61 ns	117.50 ns	13,762.9 ns	13,601.4 ns	14,015.2 ns	2.3997	0.1636	-	15216 B
ReadJson<Hashtable>	DeserializeFromUtf8Bytes	67,377.3 ns	1,659.28 ns	1,910.83 ns	67,248.2 ns	64,903.0 ns	72,845.5 ns	7.0404	1.3038	-	45282 B
ReadJson<ImmutableDictionary<String, String>>	DeserializeFromUtf8Bytes	46,786.9 ns	379.34 ns	354.84 ns	46,728.0 ns	46,191.5 ns	47,358.7 ns	4.9926	0.5547	-	32368 B
ReadJson<ImmutableSortedDictionary<String, String>>	DeserializeFromUtf8Bytes	72,652.2 ns	733.89 ns	686.48 ns	72,548.6 ns	71,654.5 ns	73,647.9 ns	6.2500	0.8523	-	39208 B
ReadJson<IndexViewModel>	DeserializeFromUtf8Bytes	29,374.9 ns	331.78 ns	294.11 ns	29,270.6 ns	29,068.4 ns	29,973.3 ns	3.4961	0.3496	-	22504 B
ReadJson<Int32>	DeserializeFromUtf8Bytes	106.3 ns	0.89 ns	0.83 ns	106.4 ns	104.7 ns	107.3 ns	-	-	-	-
ReadJson<LargeStructWithProperties>	DeserializeFromUtf8Bytes	1,130.5 ns	6.80 ns	6.36 ns	1,129.6 ns	1,122.0 ns	1,141.1 ns	0.0274	-	-	200 B
ReadJson<Location>	DeserializeFromUtf8Bytes	1,181.6 ns	10.60 ns	9.39 ns	1,180.3 ns	1,165.8 ns	1,197.6 ns	0.0683	-	-	448 B
ReadJson<LoginViewModel>	DeserializeFromUtf8Bytes	472.2 ns	9.76 ns	10.44 ns	472.9 ns	459.8 ns	496.2 ns	0.0264	-	-	168 B
ReadJson<MyEventsListerViewModel>	DeserializeFromUtf8Bytes	322,115.1 ns	2,461.36 ns	2,302.36 ns	322,283.0 ns	318,197.7 ns	326,914.3 ns	11.4213	2.5381	-	78714 B
ReadJson<SimpleStructWithProperties>	DeserializeFromUtf8Bytes	346.9 ns	20.37 ns	23.45 ns	341.9 ns	318.9 ns	394.5 ns	0.0090	-	-	64 B
ReadJson<ArrayList>	DeserializeFromStream	49,650.4 ns	642.03 ns	600.55 ns	49,468.5 ns	49,076.7 ns	51,492.8 ns	4.9300	0.5916	-	31088 B
ReadJson<BinaryData>	DeserializeFromStream	865.7 ns	11.55 ns	10.80 ns	868.0 ns	842.0 ns	879.1 ns	0.1807	-	-	1144 B
ReadJson<Dictionary<String, String>>	DeserializeFromStream	22,967.5 ns	258.54 ns	241.84 ns	23,031.3 ns	22,565.0 ns	23,358.8 ns	4.1071	0.4464	-	25944 B
ReadJson<HashSet<String>>	DeserializeFromStream	14,812.5 ns	130.13 ns	115.36 ns	14,803.2 ns	14,637.5 ns	15,067.6 ns	2.4152	0.1767	-	15288 B
ReadJson<Hashtable>	DeserializeFromStream	66,574.3 ns	1,256.13 ns	1,233.69 ns	65,985.9 ns	65,664.9 ns	69,532.9 ns	7.1694	1.0621	-	45352 B
ReadJson<ImmutableDictionary<String, String>>	DeserializeFromStream	49,008.3 ns	817.75 ns	682.86 ns	48,836.5 ns	48,221.3 ns	50,290.3 ns	5.0310	0.5805	-	32440 B
ReadJson<ImmutableSortedDictionary<String, String>>	DeserializeFromStream	81,203.2 ns	489.68 ns	458.04 ns	81,177.6 ns	80,337.4 ns	81,922.0 ns	6.1528	0.6477	-	39280 B
ReadJson<IndexViewModel>	DeserializeFromStream	32,711.6 ns	445.74 ns	416.94 ns	32,599.6 ns	32,067.0 ns	33,552.4 ns	3.5220	0.3913	-	22576 B
ReadJson<Int32>	DeserializeFromStream	303.7 ns	1.62 ns	1.51 ns	303.4 ns	301.8 ns	306.3 ns	0.0111	-	-	72 B
ReadJson<LargeStructWithProperties>	DeserializeFromStream	1,624.8 ns	15.91 ns	14.88 ns	1,624.9 ns	1,590.6 ns	1,646.4 ns	0.0516	-	-	328 B
ReadJson<Location>	DeserializeFromStream	1,612.5 ns	30.67 ns	35.32 ns	1,593.9 ns	1,572.3 ns	1,685.7 ns	0.0819	-	-	520 B
ReadJson<LoginViewModel>	DeserializeFromStream	762.4 ns	15.22 ns	16.29 ns	760.6 ns	742.5 ns	802.2 ns	0.0357	-	-	240 B
ReadJson<MyEventsListerViewModel>	DeserializeFromStream	384,195.9 ns	3,742.68 ns	3,500.90 ns	384,658.1 ns	377,496.5 ns	388,682.9 ns	12.1951	3.0488	-	79514 B
ReadJson<SimpleStructWithProperties>	DeserializeFromStream	580.4 ns	11.06 ns	10.35 ns	577.9 ns	569.0 ns	606.2 ns	0.0207	-	-	144 B

Now:

Type	Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
ReadJson<ArrayList>	DeserializeFromUtf8Bytes	48,300.3 ns	189.82 ns	177.56 ns	48,240.3 ns	48,113.4 ns	48,674.2 ns	4.8022	0.5763	-	31016 B
ReadJson<BinaryData>	DeserializeFromUtf8Bytes	507.1 ns	3.49 ns	3.10 ns	506.8 ns	502.6 ns	513.1 ns	0.1689	-	-	1072 B
ReadJson<Dictionary<String, String>>	DeserializeFromUtf8Bytes	21,058.2 ns	266.56 ns	249.34 ns	21,029.9 ns	20,728.5 ns	21,441.8 ns	4.0833	0.5833	-	25872 B
ReadJson<HashSet<String>>	DeserializeFromUtf8Bytes	14,006.4 ns	161.10 ns	142.82 ns	14,036.1 ns	13,768.6 ns	14,234.0 ns	2.4186	0.1649	-	15216 B
ReadJson<Hashtable>	DeserializeFromUtf8Bytes	65,265.2 ns	282.61 ns	250.53 ns	65,318.3 ns	64,776.7 ns	65,632.7 ns	7.0922	1.3134	-	45280 B
ReadJson<ImmutableDictionary<String, String>>	DeserializeFromUtf8Bytes	46,677.9 ns	417.39 ns	390.42 ns	46,621.7 ns	46,143.0 ns	47,459.3 ns	4.9779	0.5531	-	32368 B
ReadJson<ImmutableSortedDictionary<String, String>>	DeserializeFromUtf8Bytes	77,860.1 ns	498.28 ns	416.09 ns	77,809.6 ns	77,239.0 ns	78,895.8 ns	6.1881	0.9282	-	39208 B
ReadJson<IndexViewModel>	DeserializeFromUtf8Bytes	29,217.9 ns	235.41 ns	220.20 ns	29,218.2 ns	28,812.0 ns	29,689.7 ns	3.4961	0.3496	-	22504 B
ReadJson<Int32>	DeserializeFromUtf8Bytes	104.8 ns	0.52 ns	0.46 ns	104.7 ns	104.1 ns	105.5 ns	-	-	-	-
ReadJson<LargeStructWithProperties>	DeserializeFromUtf8Bytes	1,164.1 ns	7.07 ns	6.27 ns	1,161.5 ns	1,156.4 ns	1,176.9 ns	0.0278	-	-	200 B
ReadJson<Location>	DeserializeFromUtf8Bytes	1,194.4 ns	9.27 ns	7.74 ns	1,197.2 ns	1,173.8 ns	1,201.6 ns	0.0674	-	-	448 B
ReadJson<LoginViewModel>	DeserializeFromUtf8Bytes	455.1 ns	6.87 ns	6.09 ns	455.5 ns	446.2 ns	465.6 ns	0.0255	-	-	168 B
ReadJson<MyEventsListerViewModel>	DeserializeFromUtf8Bytes	326,594.4 ns	2,843.42 ns	2,520.62 ns	326,530.1 ns	321,413.7 ns	331,308.2 ns	11.7801	2.6178	-	78714 B
ReadJson<SimpleStructWithProperties>	DeserializeFromUtf8Bytes	324.4 ns	1.98 ns	1.85 ns	324.4 ns	320.8 ns	327.2 ns	0.0091	-	-	64 B
ReadJson<ArrayList>	DeserializeFromStream	52,083.5 ns	418.37 ns	370.87 ns	52,218.8 ns	51,274.4 ns	52,500.3 ns	4.7827	0.6238	-	31090 B
ReadJson<BinaryData>	DeserializeFromStream	844.1 ns	9.87 ns	9.24 ns	846.1 ns	824.4 ns	852.7 ns	0.1813	-	-	1144 B
ReadJson<Dictionary<String, String>>	DeserializeFromStream	22,176.6 ns	232.96 ns	217.91 ns	22,113.5 ns	21,935.3 ns	22,521.4 ns	4.0607	0.4414	-	25944 B
ReadJson<HashSet<String>>	DeserializeFromStream	14,867.0 ns	138.48 ns	129.53 ns	14,813.8 ns	14,709.5 ns	15,096.5 ns	2.4243	0.1774	-	15288 B
ReadJson<Hashtable>	DeserializeFromStream	66,353.0 ns	464.60 ns	434.59 ns	66,439.5 ns	65,641.0 ns	66,991.6 ns	7.1828	1.0641	-	45352 B
ReadJson<ImmutableDictionary<String, String>>	DeserializeFromStream	50,086.9 ns	460.06 ns	430.34 ns	50,057.0 ns	49,222.7 ns	50,798.6 ns	5.0896	0.6107	-	32440 B
ReadJson<ImmutableSortedDictionary<String, String>>	DeserializeFromStream	76,215.1 ns	724.63 ns	677.82 ns	76,110.2 ns	75,414.1 ns	77,506.5 ns	6.1576	0.6158	-	39280 B
ReadJson<IndexViewModel>	DeserializeFromStream	32,288.7 ns	313.06 ns	292.84 ns	32,270.4 ns	31,832.4 ns	32,740.0 ns	3.4709	0.3857	-	22576 B
ReadJson<Int32>	DeserializeFromStream	300.0 ns	3.40 ns	3.18 ns	298.3 ns	296.4 ns	305.5 ns	0.0107	-	-	72 B
ReadJson<LargeStructWithProperties>	DeserializeFromStream	1,671.6 ns	14.93 ns	13.23 ns	1,671.6 ns	1,648.6 ns	1,700.8 ns	0.0463	-	-	328 B
ReadJson<Location>	DeserializeFromStream	1,593.5 ns	14.26 ns	13.34 ns	1,593.2 ns	1,570.1 ns	1,612.3 ns	0.0823	-	-	520 B
ReadJson<LoginViewModel>	DeserializeFromStream	753.0 ns	8.09 ns	7.57 ns	754.8 ns	743.3 ns	770.6 ns	0.0361	-	-	240 B
ReadJson<MyEventsListerViewModel>	DeserializeFromStream	377,660.1 ns	4,094.25 ns	3,829.76 ns	378,626.5 ns	372,579.7 ns	384,137.9 ns	12.2324	3.0581	-	79515 B
ReadJson<SimpleStructWithProperties>	DeserializeFromStream	558.0 ns	4.21 ns	3.94 ns	558.4 ns	552.0 ns	564.1 ns	0.0222	-	-	144 B

summary

Type	Method	before/now (mean)
ReadJson<ArrayList>	DeserializeFromUtf8Bytes	0.954023465
ReadJson<BinaryData>	DeserializeFromUtf8Bytes	0.969969396
ReadJson<Dictionary<String, String>>	DeserializeFromUtf8Bytes	1.0274899
ReadJson<HashSet>	DeserializeFromUtf8Bytes	1.017810817
ReadJson<Hashtable>	DeserializeFromUtf8Bytes	0.968652647
ReadJson<ImmutableDictionary<String, String>>	DeserializeFromUtf8Bytes	0.997670288
ReadJson<ImmutableSortedDictionary<String, String>>	DeserializeFromUtf8Bytes	1.071682619
ReadJson<IndexViewModel>	DeserializeFromUtf8Bytes	0.994655301
ReadJson<Int32>	DeserializeFromUtf8Bytes	0.985888993
ReadJson<LargeStructWithProperties>	DeserializeFromUtf8Bytes	1.029721362
ReadJson<Location>	DeserializeFromUtf8Bytes	1.010832769
ReadJson<LoginViewModel>	DeserializeFromUtf8Bytes	0.963786531
ReadJson<MyEventsListerViewModel>	DeserializeFromUtf8Bytes	1.013905899
ReadJson<SimpleStructWithProperties>	DeserializeFromUtf8Bytes	0.93513981
ReadJson<ArrayList>	DeserializeFromStream	1.04900464
ReadJson<BinaryData>	DeserializeFromStream	0.975049093
ReadJson<Dictionary<String, String>>	DeserializeFromStream	0.965564384
ReadJson<HashSet>	DeserializeFromStream	1.003679325
ReadJson<Hashtable>	DeserializeFromStream	0.996675894
ReadJson<ImmutableDictionary<String, String>>	DeserializeFromStream	1.022008517
ReadJson<ImmutableSortedDictionary<String, String>>	DeserializeFromStream	0.938572618
ReadJson<IndexViewModel>	DeserializeFromStream	0.987071864
ReadJson<Int32>	DeserializeFromStream	0.987816925
ReadJson<LargeStructWithProperties>	DeserializeFromStream	1.028803545
ReadJson<Location>	DeserializeFromStream	0.988217054
ReadJson<LoginViewModel>	DeserializeFromStream	0.987670514
ReadJson<MyEventsListerViewModel>	DeserializeFromStream	0.982988366
ReadJson<SimpleStructWithProperties>	DeserializeFromStream	0.961405927

Write

results

Before:

Type	Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
WriteJson<IndexViewModel>	SerializeToUtf8Bytes	17,090.9 ns	124.41 ns	116.37 ns	17,089.0 ns	16,954.5 ns	17,323.3 ns	2.0650	0.0688	-	13016 B
WriteJson<Int32>	SerializeToUtf8Bytes	144.6 ns	1.48 ns	1.39 ns	145.1 ns	141.2 ns	145.9 ns	0.0290	-	-	184 B
WriteJson<Location>	SerializeToUtf8Bytes	843.9 ns	10.49 ns	9.82 ns	849.4 ns	824.7 ns	855.3 ns	0.0600	-	-	384 B
WriteJson<LoginViewModel>	SerializeToUtf8Bytes	343.4 ns	5.18 ns	4.85 ns	342.5 ns	335.8 ns	352.9 ns	0.0421	-	-	264 B
WriteJson<MyEventsListerViewModel>	SerializeToUtf8Bytes	431,276.0 ns	2,504.57 ns	2,342.78 ns	431,172.6 ns	426,409.1 ns	434,945.3 ns	28.7162	5.0676	-	191432 B
WriteJson<IndexViewModel>	SerializeToStream	16,731.5 ns	183.90 ns	172.02 ns	16,755.3 ns	16,509.8 ns	16,959.0 ns	0.0667	-	-	432 B
WriteJson<Int32>	SerializeToStream	226.1 ns	1.63 ns	1.52 ns	225.7 ns	223.7 ns	229.3 ns	0.0240	-	-	152 B
WriteJson<Location>	SerializeToStream	940.7 ns	11.58 ns	10.83 ns	944.3 ns	923.8 ns	954.1 ns	0.0229	-	-	152 B
WriteJson<LoginViewModel>	SerializeToStream	430.9 ns	2.42 ns	2.02 ns	431.4 ns	426.7 ns	433.1 ns	0.0227	-	-	152 B
WriteJson<MyEventsListerViewModel>	SerializeToStream	415,983.5 ns	3,423.78 ns	3,202.61 ns	414,854.8 ns	412,741.9 ns	422,329.6 ns	18.0921	-	-	116104 B

Now:

Type	Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
WriteJson<IndexViewModel>	SerializeToUtf8Bytes	16,909.1 ns	62.43 ns	52.13 ns	16,926.6 ns	16,832.1 ns	17,002.1 ns	2.0336	0.0678	-	13016 B
WriteJson<Int32>	SerializeToUtf8Bytes	141.1 ns	1.11 ns	0.98 ns	141.1 ns	139.2 ns	142.9 ns	0.0289	-	-	184 B
WriteJson<Location>	SerializeToUtf8Bytes	883.7 ns	8.48 ns	7.93 ns	885.5 ns	868.6 ns	894.1 ns	0.0608	-	-	384 B
WriteJson<LoginViewModel>	SerializeToUtf8Bytes	347.4 ns	8.39 ns	9.66 ns	347.6 ns	332.0 ns	367.8 ns	0.0408	-	-	264 B
WriteJson<MyEventsListerViewModel>	SerializeToUtf8Bytes	421,477.8 ns	5,711.71 ns	5,342.74 ns	421,054.7 ns	414,107.3 ns	430,927.0 ns	28.7162	5.0676	-	191432 B
WriteJson<IndexViewModel>	SerializeToStream	17,151.0 ns	196.95 ns	184.22 ns	17,208.2 ns	16,784.8 ns	17,387.3 ns	0.0670	-	-	432 B
WriteJson<Int32>	SerializeToStream	214.7 ns	2.22 ns	2.08 ns	215.0 ns	211.8 ns	218.3 ns	0.0239	-	-	152 B
WriteJson<Location>	SerializeToStream	922.5 ns	8.25 ns	7.31 ns	922.2 ns	912.4 ns	936.1 ns	0.0203	-	-	152 B
WriteJson<LoginViewModel>	SerializeToStream	437.0 ns	2.23 ns	2.09 ns	436.5 ns	435.0 ns	441.0 ns	0.0227	-	-	152 B
WriteJson<MyEventsListerViewModel>	SerializeToStream	408,500.2 ns	2,773.06 ns	2,458.25 ns	408,301.8 ns	405,188.2 ns	413,476.8 ns	18.0921	-	-	116104 B

summary

Type	Method	before/now (mean)
WriteJson<IndexViewModel>	SerializeToUtf8Bytes	0.98936276
WriteJson<Int32>	SerializeToUtf8Bytes	0.975795297
WriteJson<Location>	SerializeToUtf8Bytes	1.047161986
WriteJson<LoginViewModel>	SerializeToUtf8Bytes	1.011648224
WriteJson<MyEventsListerViewModel>	SerializeToUtf8Bytes	0.977280906
WriteJson<IndexViewModel>	SerializeToStream	1.025072468
WriteJson<Int32>	SerializeToStream	0.949579832
WriteJson<Location>	SerializeToStream	0.980652705
WriteJson<LoginViewModel>	SerializeToStream	1.014156417
WriteJson<MyEventsListerViewModel>	SerializeToStream	0.982010585

…ndler_1

jozkee · 2020-05-29T19:06:54Z

For ReferenceHandler.Preserve, allocation increased by 40 bytes on read/write, this is because we replaced struct DefaultReferenceResolver forabstract class ReferenceResolver:

Read

results

Before:

Type	Method	IsDataPreserved	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
ReadPreservedReferences<IndexViewModel>	DeserializePreserved	False	32,546.9 ns	240.64 ns	187.87 ns	32,571.3 ns	32,255.5 ns	32,925.9 ns	3.5266	0.3918	-	22584 B
ReadPreservedReferences<Location>	DeserializePreserved	False	1,310.9 ns	17.94 ns	16.78 ns	1,313.0 ns	1,284.2 ns	1,339.3 ns	0.0830	-	-	528 B
ReadPreservedReferences<LoginViewModel>	DeserializePreserved	False	588.4 ns	5.21 ns	4.87 ns	588.8 ns	579.7 ns	596.5 ns	0.0379	-	-	248 B
ReadPreservedReferences<MyEventsListerViewModel>	DeserializePreserved	False	361,033.0 ns	4,439.66 ns	4,152.86 ns	361,530.5 ns	352,696.0 ns	366,774.5 ns	11.3314	2.8329	-	78794 B
ReadPreservedReferences<SimpleListOfInt>	DeserializePreserved	False	396.1 ns	5.82 ns	5.44 ns	396.8 ns	384.0 ns	403.6 ns	0.0236	-	-	152 B
ReadPreservedReferences<SimpleStructWithProperties>	DeserializePreserved	False	451.5 ns	5.11 ns	4.27 ns	452.3 ns	441.8 ns	459.3 ns	0.0216	-	-	144 B
ReadPreservedReferences<IndexViewModel>	DeserializePreserved	True	8,859.8 ns	145.38 ns	135.99 ns	8,858.2 ns	8,681.5 ns	9,070.7 ns	0.6780	-	-	4376 B
ReadPreservedReferences<Location>	DeserializePreserved	True	1,481.3 ns	19.90 ns	17.64 ns	1,476.5 ns	1,452.2 ns	1,517.2 ns	0.1117	-	-	720 B
ReadPreservedReferences<LoginViewModel>	DeserializePreserved	True	768.0 ns	8.13 ns	7.60 ns	767.2 ns	757.8 ns	783.0 ns	0.0672	-	-	440 B
ReadPreservedReferences<MyEventsListerViewModel>	DeserializePreserved	True	32,256.7 ns	345.99 ns	306.71 ns	32,171.2 ns	31,833.3 ns	32,762.1 ns	1.6682	-	-	10808 B
ReadPreservedReferences<SimpleListOfInt>	DeserializePreserved	True	641.5 ns	8.35 ns	7.81 ns	641.4 ns	628.3 ns	654.0 ns	0.0591	-	-	376 B
ReadPreservedReferences<SimpleStructWithProperties>	DeserializePreserved	True	566.1 ns	10.82 ns	10.13 ns	568.0 ns	542.1 ns	579.8 ns	0.0521	-	-	336 B

Now:

Type	Method	IsDataPreserved	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
ReadPreservedReferences<IndexViewModel>	DeserializePreserved	False	32,589.4 ns	347.58 ns	325.12 ns	32,473.2 ns	32,165.2 ns	33,162.1 ns	3.5129	0.3903	-	22624 B
ReadPreservedReferences<Location>	DeserializePreserved	False	1,377.8 ns	22.32 ns	20.88 ns	1,383.5 ns	1,343.8 ns	1,411.6 ns	0.0867	-	-	568 B
ReadPreservedReferences<LoginViewModel>	DeserializePreserved	False	589.7 ns	10.34 ns	9.67 ns	591.8 ns	571.9 ns	602.4 ns	0.0437	-	-	288 B
ReadPreservedReferences<MyEventsListerViewModel>	DeserializePreserved	False	353,784.3 ns	6,302.89 ns	5,895.72 ns	352,694.7 ns	344,959.5 ns	364,336.1 ns	12.5348	2.7855	-	78834 B
ReadPreservedReferences<SimpleListOfInt>	DeserializePreserved	False	411.6 ns	5.51 ns	4.60 ns	413.1 ns	402.7 ns	417.0 ns	0.0294	-	-	192 B
ReadPreservedReferences<SimpleStructWithProperties>	DeserializePreserved	False	459.5 ns	2.42 ns	1.89 ns	459.0 ns	457.1 ns	463.3 ns	0.0278	-	-	184 B
ReadPreservedReferences<IndexViewModel>	DeserializePreserved	True	9,084.4 ns	127.47 ns	119.23 ns	9,056.0 ns	8,918.7 ns	9,353.3 ns	0.6912	-	-	4416 B
ReadPreservedReferences<Location>	DeserializePreserved	True	1,468.2 ns	22.17 ns	20.74 ns	1,472.0 ns	1,437.0 ns	1,506.6 ns	0.1158	-	-	760 B
ReadPreservedReferences<LoginViewModel>	DeserializePreserved	True	734.9 ns	6.90 ns	6.45 ns	735.3 ns	723.7 ns	746.5 ns	0.0737	-	-	480 B
ReadPreservedReferences<MyEventsListerViewModel>	DeserializePreserved	True	30,707.2 ns	529.17 ns	494.99 ns	30,735.9 ns	30,036.5 ns	31,554.8 ns	1.7155	-	-	10848 B
ReadPreservedReferences<SimpleListOfInt>	DeserializePreserved	True	637.7 ns	7.28 ns	6.81 ns	638.0 ns	628.3 ns	649.6 ns	0.0655	-	-	416 B
ReadPreservedReferences<SimpleStructWithProperties>	DeserializePreserved	True	575.6 ns	4.76 ns	4.22 ns	575.9 ns	565.7 ns	583.4 ns	0.0581	-	-	376 B

summary

type	IsDataPreserved	before/now
ReadPreservedReferences<IndexViewModel>	FALSE	1.001305808
ReadPreservedReferences<Location>	FALSE	1.051033641
ReadPreservedReferences<LoginViewModel>	FALSE	1.002209381
ReadPreservedReferences<MyEventsListerViewModel>	FALSE	0.979922334
ReadPreservedReferences<SimpleListOfInt>	FALSE	1.039131532
ReadPreservedReferences<SimpleStructWithProperties>	FALSE	1.017718715
ReadPreservedReferences<IndexViewModel>	TRUE	1.025350459
ReadPreservedReferences<Location>	TRUE	0.991156417
ReadPreservedReferences<LoginViewModel>	TRUE	0.956901042
ReadPreservedReferences<MyEventsListerViewModel>	TRUE	0.951963468
ReadPreservedReferences<SimpleListOfInt>	TRUE	0.994076383
ReadPreservedReferences<SimpleStructWithProperties>	TRUE	1.016781487

Write

results

Before:

Type	Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
WritePreservedReferences<IndexViewModel>	SerializePreserved	4,368.0 ns	41.35 ns	38.68 ns	4,357.7 ns	4,320.2 ns	4,446.1 ns	0.5349	-	-	3408 B
WritePreservedReferences<Location>	SerializePreserved	1,025.4 ns	9.01 ns	7.99 ns	1,025.7 ns	1,011.7 ns	1,039.9 ns	0.1280	-	-	816 B
WritePreservedReferences<LoginViewModel>	SerializePreserved	492.0 ns	5.25 ns	4.65 ns	491.3 ns	483.7 ns	501.7 ns	0.0913	-	-	576 B
WritePreservedReferences<MyEventsListerViewModel>	SerializePreserved	16,863.4 ns	71.32 ns	55.68 ns	16,876.4 ns	16,741.8 ns	16,938.8 ns	1.2760	-	-	8312 B
WritePreservedReferences<SimpleListOfInt>	SerializePreserved	395.6 ns	5.79 ns	5.42 ns	394.3 ns	389.4 ns	404.5 ns	0.0721	-	-	456 B
WritePreservedReferences<SimpleStructWithProperties>	SerializePreserved	330.2 ns	3.15 ns	2.95 ns	330.2 ns	327.0 ns	337.3 ns	0.0531	-	-	336 B

Now:

Type	Method	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
WritePreservedReferences<IndexViewModel>	SerializePreserved	4,499.9 ns	60.00 ns	56.12 ns	4,492.3 ns	4,389.6 ns	4,590.0 ns	0.5380	-	-	3448 B
WritePreservedReferences<Location>	SerializePreserved	1,026.2 ns	8.86 ns	8.29 ns	1,027.2 ns	1,011.8 ns	1,037.0 ns	0.1335	-	-	856 B
WritePreservedReferences<LoginViewModel>	SerializePreserved	502.8 ns	3.98 ns	3.72 ns	502.1 ns	498.8 ns	510.1 ns	0.0976	-	-	616 B
WritePreservedReferences<MyEventsListerViewModel>	SerializePreserved	17,254.6 ns	183.38 ns	162.57 ns	17,297.9 ns	16,977.0 ns	17,468.3 ns	1.2881	-	-	8360 B
WritePreservedReferences<SimpleListOfInt>	SerializePreserved	386.4 ns	6.36 ns	5.95 ns	386.4 ns	376.8 ns	397.3 ns	0.0784	-	-	496 B
WritePreservedReferences<SimpleStructWithProperties>	SerializePreserved	332.4 ns	4.31 ns	4.03 ns	331.7 ns	326.0 ns	339.5 ns	0.0589	-	-	376 B

summary

type	before/now
WritePreservedReferences<IndexViewModel>	1.030196886
WritePreservedReferences<Location>	1.000780183
WritePreservedReferences<LoginViewModel>	1.02195122
WritePreservedReferences<MyEventsListerViewModel>	1.023198169
WritePreservedReferences<SimpleListOfInt>	0.976744186
WritePreservedReferences<SimpleStructWithProperties>	1.006662629

steveharter · 2020-06-01T19:19:36Z

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

-            bool shouldReadPreservedReferences = options.ReferenceHandling.ShouldReadPreservedReferences();
-
-            if (!state.SupportContinuation && !shouldReadPreservedReferences)
+            if (!state.SupportContinuation && options.ReferenceHandler == null)


It looks like these 2 variables are always checked together. Although they could be separate in the future, consider collapsing then having state.SupportContinuation consider options.ReferencHandler factored in to prevent the &&

As of today, state.SupportContinuation and options.ReferenceHandler need to go through the "slow path" since both relies on the state.Current.ObjectState.

Is your suggestion related to refactor the code so we can do a fast path for ReferenceHandler when there is no need for continuation? If that's the case I can try to tackle that in a follow-up PR.

Not a fast path. Just having a single bool like
state.UseFastPath = !state.SupportContinuation && options.ReferenceHandler == null

steveharter · 2020-06-01T19:33:34Z

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


                // Handle the metadata properties.
-                if (shouldReadPreservedReferences && state.Current.ObjectState < StackFrameObjectState.PropertyValue)
+                if (options.ReferenceHandler != null && state.Current.ObjectState < StackFrameObjectState.PropertyValue)


Cache these in a bool (since accessed elsewhere in same method)?

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

steveharter · 2020-06-01T19:48:37Z

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

+    internal sealed class PreserveReferenceResolver : ReferenceResolver
+    {
+        private uint _referenceCount;
+        private readonly Dictionary<string, object>? _referenceIdToObjectMap;


I assume ConcurrentDictionary is not used is because we create a new instance for every (de)serialization call -- correct? i.e. we don't have to worry about multiple threads

That's correct. We call CreateConverter that returns a new instance every time we (de)serialize unless someone provides its own ReferenceHandler implementation that does otherwise.

src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.cs

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

…dler (dotnet#36829) * Expose ReferenceResolver and rename ReferenceHandling to ReferenceHandler * Address some feedback * Address feedback * Clean-up code * Change messages in string.resx * Add test for a badly implemented resolver * Address feedback.

…dler (#36829) (#37296) * Expose ReferenceResolver and rename ReferenceHandling to ReferenceHandler * Address some feedback * Address feedback * Clean-up code * Change messages in string.resx * Add test for a badly implemented resolver * Address feedback.

Follow up to dotnet/runtime#37296 (comment) The property and the type ReferenceHandling were recently renamed to Referencehandler on dotnet/runtime#36829 Above PR was also ported to dotnet/runtime preview6 branch under dotnet/runtime#37296 therefore once this is into master, it should be ported to release/5.0-preview6 branch as well.

jozkee added the area-System.Text.Json label May 21, 2020

jozkee added this to the 5.0 milestone May 21, 2020

jozkee requested review from layomia and steveharter May 21, 2020 19:02

jozkee self-assigned this May 21, 2020

Dotnet-GitSync-Bot added the new-api-needs-documentation label May 21, 2020

jozkee commented May 21, 2020

View reviewed changes

layomia reviewed May 21, 2020

View reviewed changes

jaredpar mentioned this pull request May 22, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

jozkee added 3 commits May 27, 2020 11:35

Expose ReferenceResolver and rename ReferenceHandling to ReferenceHan…

0749a18

…dler

Address some feedback

a2fbfd0

Address feedback

bdd8aa2

jozkee force-pushed the refhandler_1 branch from e032ccf to bdd8aa2 Compare May 27, 2020 18:38

jozkee added 4 commits May 29, 2020 11:14

Clean-up code

ee810c2

Change messages in string.resx

4c3f72e

Add test for a badly implemented resolver

fc50a0c

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

564cb7f

…ndler_1

jozkee requested a review from layomia May 29, 2020 18:44

steveharter reviewed Jun 1, 2020

View reviewed changes

...es/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleMetadata.cs Outdated Show resolved Hide resolved

steveharter reviewed Jun 1, 2020

View reviewed changes

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

layomia approved these changes Jun 1, 2020

View reviewed changes

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

Address feedback.

40ddda9

layomia mentioned this pull request Jun 2, 2020

Automatically port 5.0 System.Text.Json documentation dotnet/dotnet-api-docs#4284

Merged

jozkee merged commit 3f776fb into dotnet:master Jun 2, 2020

This was referenced Jun 2, 2020

[Release/5.0-preview6] Expose ReferenceResolver and rename ReferenceHandling to ReferenceHandler #37296

Merged

Fix build issue related to rename of ReferenceHandler dotnet/performance#1342

Merged

akoeplinger mentioned this pull request Jun 2, 2020

dotnet-runtime-perf CI job is failing due to renamed System.Text.Json.Serialization.ReferenceHandling type #37319

Closed

This was referenced Jun 2, 2020

Update JSON tests to use new property name dotnet/performance#1343

Closed

Remove exclude paths for libraries and installer #37316

Merged

jozkee mentioned this pull request Jun 4, 2020

Update references to ReferenceHandler based on recent renaming dotnet/efcore#21135

Closed

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

jozkee deleted the refhandler_1 branch April 16, 2024 15:48

jozkee restored the refhandler_1 branch April 16, 2024 15:48

jozkee deleted the refhandler_1 branch April 16, 2024 15:49

Expose ReferenceResolver and rename ReferenceHandling to ReferenceHandler #36829

Expose ReferenceResolver and rename ReferenceHandling to ReferenceHandler #36829

Uh oh!

Conversation

jozkee commented May 21, 2020

Uh oh!

Dotnet-GitSync-Bot commented May 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

layomia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jozkee May 22, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jozkee commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Read

Write

Uh oh!

jozkee commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Read

Write

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jozkee May 22, 2020 •

edited

Loading

jozkee commented May 29, 2020 •

edited

Loading

jozkee commented May 29, 2020 •

edited

Loading