From db3adc8935326ad6bb1dacfc629556dd7f48ba98 Mon Sep 17 00:00:00 2001 From: Layomi Akinrinade Date: Sat, 28 Mar 2020 15:46:21 -0700 Subject: [PATCH 1/3] Cache parameterized ctor delegates in class info rather than converter --- ...ParameterizedConstructorConverter.Large.cs | 12 +-- ...ParameterizedConstructorConverter.Small.cs | 10 +- .../Text/Json/Serialization/JsonClassInfo.cs | 10 +- .../Text/Json/Serialization/JsonConverter.cs | 2 +- .../Serialization/ConstructorTests.Cache.cs | 67 +++++++------ .../ConstructorTests.ParameterMatching.cs | 9 +- .../Serialization/DeserializationWrapper.cs | 31 ++++++- .../Serialization/TestClasses.Constructor.cs | 93 +++++++++++++++---- 8 files changed, 167 insertions(+), 67 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs index f4fabaf4a588c..f62766a24abcc 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs @@ -12,11 +12,9 @@ namespace System.Text.Json.Serialization.Converters /// internal sealed class LargeObjectWithParameterizedConstructorConverter : ObjectWithParameterizedConstructorConverter where T : notnull { - private JsonClassInfo.ParameterizedConstructorDelegate? _createObject; - - internal override void CreateConstructorDelegate(JsonSerializerOptions options) + internal override void CreateConstructorDelegate(JsonClassInfo classInfo, JsonSerializerOptions options) { - _createObject = options.MemberAccessorStrategy.CreateParameterizedConstructor(ConstructorInfo)!; + classInfo.CreateObjectWithParameterizedCtor = options.MemberAccessorStrategy.CreateParameterizedConstructor(ConstructorInfo)!; } protected override bool ReadAndCacheConstructorArgument(ref ReadStack state, ref Utf8JsonReader reader, JsonParameterInfo jsonParameterInfo) @@ -35,13 +33,15 @@ protected override object CreateObject(ref ReadStackFrame frame) { object[] arguments = (object[])frame.CtorArgumentState!.Arguments!; - if (_createObject == null) + var createObject = (JsonClassInfo.ParameterizedConstructorDelegate)frame.JsonClassInfo.CreateObjectWithParameterizedCtor!; + + if (createObject == null) { // This means this constructor has more than 64 parameters. ThrowHelper.ThrowNotSupportedException_ConstructorMaxOf64Parameters(ConstructorInfo, TypeToConvert); } - object obj = _createObject(arguments)!; + object obj = createObject(arguments)!; ArrayPool.Shared.Return(arguments, clearArray: true); return obj; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs index fb852b81ad88d..70afd39f9c14a 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs @@ -12,17 +12,17 @@ namespace System.Text.Json.Serialization.Converters /// internal sealed class SmallObjectWithParameterizedConstructorConverter : ObjectWithParameterizedConstructorConverter where T : notnull { - private JsonClassInfo.ParameterizedConstructorDelegate? _createObject; - - internal override void CreateConstructorDelegate(JsonSerializerOptions options) + internal override void CreateConstructorDelegate(JsonClassInfo classInfo, JsonSerializerOptions options) { - _createObject = options.MemberAccessorStrategy.CreateParameterizedConstructor(ConstructorInfo)!; + classInfo.CreateObjectWithParameterizedCtor = options.MemberAccessorStrategy.CreateParameterizedConstructor(ConstructorInfo)!; } protected override object CreateObject(ref ReadStackFrame frame) { + var createObject = (JsonClassInfo.ParameterizedConstructorDelegate) + frame.JsonClassInfo.CreateObjectWithParameterizedCtor!; var arguments = (Arguments)frame.CtorArgumentState!.Arguments!; - return _createObject!(arguments.Arg0, arguments.Arg1, arguments.Arg2, arguments.Arg3)!; + return createObject!(arguments.Arg0, arguments.Arg1, arguments.Arg2, arguments.Arg3)!; } protected override bool ReadAndCacheConstructorArgument(ref ReadStack state, ref Utf8JsonReader reader, JsonParameterInfo jsonParameterInfo) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs index 3068699eac2f2..48d220efc6296 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs @@ -20,6 +20,8 @@ internal sealed partial class JsonClassInfo public ConstructorDelegate? CreateObject { get; private set; } + public object? CreateObjectWithParameterizedCtor { get; set; } + public ClassType ClassType { get; private set; } public JsonPropertyInfo? DataExtensionProperty { get; private set; } @@ -159,7 +161,13 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) if (converter.ConstructorIsParameterized) { - converter.CreateConstructorDelegate(options); + // Create and cache the constructor delegate for this type. + // We depend on the first converter created for this type to perform the initialization. + // Subsequent instances of the converter will not perform this initialization since we will + // not go down this code path (as the type will already be in the JsonClassInfo cache). + // The converter is used because it has generic type support. + converter.CreateConstructorDelegate(this, options); + InitializeConstructorParameters(converter.ConstructorInfo); } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs index be1cbd5650f96..98cabd6ef65ae 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs @@ -80,6 +80,6 @@ internal bool ShouldFlush(Utf8JsonWriter writer, ref WriteStack state) internal ConstructorInfo ConstructorInfo { get; set; } = null!; - internal virtual void CreateConstructorDelegate(JsonSerializerOptions options) { } + internal virtual void CreateConstructorDelegate(JsonClassInfo classInfo, JsonSerializerOptions options) { } } } diff --git a/src/libraries/System.Text.Json/tests/Serialization/ConstructorTests.Cache.cs b/src/libraries/System.Text.Json/tests/Serialization/ConstructorTests.Cache.cs index f8797d9687964..92cabd0120124 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/ConstructorTests.Cache.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/ConstructorTests.Cache.cs @@ -24,53 +24,66 @@ public void MultipleThreadsLooping() [Fact] public void MultipleThreads() { - // Use local options to avoid obtaining already cached metadata from the default options. - var options = new JsonSerializerOptions(); - // Verify the test class has >32 properties since that is a threshold for using the fallback dictionary. Assert.True(typeof(ClassWithConstructor_SimpleAndComplexParameters).GetProperties(BindingFlags.Instance | BindingFlags.Public).Length > 32); - void DeserializeObjectMinimal() + void DeserializeObject(string json, Type type, JsonSerializerOptions options) + { + var obj = Serializer.Deserialize(json, type, options); + ((ITestClassWithParameterizedCtor)obj).Verify(); + } + + void DeserializeObjectMinimal(Type type, JsonSerializerOptions options) { - var obj = Serializer.Deserialize(@"{""MyDecimal"" : 3.3}", options); + string json = (string)type.GetProperty("s_json_minimal").GetValue(null); + var obj = Serializer.Deserialize(json, type, options); + ((ITestClassWithParameterizedCtor)obj).VerifyMinimal(); }; - void DeserializeObjectFlipped() + void DeserializeObjectFlipped(Type type, JsonSerializerOptions options) { - var obj = Serializer.Deserialize( - ClassWithConstructor_SimpleAndComplexParameters.s_json_flipped, options); - obj.Verify(); + string json = (string)type.GetProperty("s_json_flipped").GetValue(null); + DeserializeObject(json, type, options); }; - void DeserializeObjectNormal() + void DeserializeObjectNormal(Type type, JsonSerializerOptions options) { - var obj = Serializer.Deserialize( - ClassWithConstructor_SimpleAndComplexParameters.s_json, options); - obj.Verify(); + string json = (string)type.GetProperty("s_json").GetValue(null); + DeserializeObject(json, type, options); }; - void SerializeObject() + void SerializeObject(Type type, JsonSerializerOptions options) { var obj = ClassWithConstructor_SimpleAndComplexParameters.GetInstance(); JsonSerializer.Serialize(obj, options); }; - const int ThreadCount = 8; - const int ConcurrentTestsCount = 4; - Task[] tasks = new Task[ThreadCount * ConcurrentTestsCount]; - - for (int i = 0; i < tasks.Length; i += ConcurrentTestsCount) + void RunTest(Type type) { - // Create race condition to populate the sorted property cache with different json ordering. - tasks[i + 0] = Task.Run(() => DeserializeObjectMinimal()); - tasks[i + 1] = Task.Run(() => DeserializeObjectFlipped()); - tasks[i + 2] = Task.Run(() => DeserializeObjectNormal()); + // Use local options to avoid obtaining already cached metadata from the default options. + var options = new JsonSerializerOptions(); - // Ensure no exceptions on serialization - tasks[i + 3] = Task.Run(() => SerializeObject()); - }; + const int ThreadCount = 8; + const int ConcurrentTestsCount = 4; + Task[] tasks = new Task[ThreadCount * ConcurrentTestsCount]; + + for (int i = 0; i < tasks.Length; i += ConcurrentTestsCount) + { + // Create race condition to populate the sorted property cache with different json ordering. + tasks[i + 0] = Task.Run(() => DeserializeObjectMinimal(type, options)); + tasks[i + 1] = Task.Run(() => DeserializeObjectFlipped(type, options)); + tasks[i + 2] = Task.Run(() => DeserializeObjectNormal(type, options)); + + // Ensure no exceptions on serialization + tasks[i + 3] = Task.Run(() => SerializeObject(type, options)); + }; + + Task.WaitAll(tasks); + } - Task.WaitAll(tasks); + RunTest(typeof(ClassWithConstructor_SimpleAndComplexParameters)); + RunTest(typeof(Person_Class)); + RunTest(typeof(Parameterized_Class_With_ComplexTuple)); } [Fact] diff --git a/src/libraries/System.Text.Json/tests/Serialization/ConstructorTests.ParameterMatching.cs b/src/libraries/System.Text.Json/tests/Serialization/ConstructorTests.ParameterMatching.cs index 3f18845236380..f6979cf22c967 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/ConstructorTests.ParameterMatching.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/ConstructorTests.ParameterMatching.cs @@ -8,14 +8,14 @@ namespace System.Text.Json.Serialization.Tests { - public class ConstructorTests_StringTValue : ConstructorTests + public class ConstructorTests_String : ConstructorTests { - public ConstructorTests_StringTValue() : base(DeserializationWrapper.StringTValueSerializer) { } + public ConstructorTests_String() : base(DeserializationWrapper.StringDeserializer) { } } - public class ConstructorTests_StreamTValue : ConstructorTests + public class ConstructorTests_Stream : ConstructorTests { - public ConstructorTests_StreamTValue() : base(DeserializationWrapper.StreamTValueSerializer) { } + public ConstructorTests_Stream() : base(DeserializationWrapper.StreamDeserializer) { } } public abstract partial class ConstructorTests @@ -301,7 +301,6 @@ public void Null_AsArgument_To_ParameterThat_CanNotBeNull() Assert.Throws(() => Serializer.Deserialize(@"{""MyPoint3DStruct"":null,""MyString"":""1""}")); } - [ActiveIssue("https://github.com/dotnet/runtime/issues/33928")] [Fact] public void OtherPropertiesAreSet() { diff --git a/src/libraries/System.Text.Json/tests/Serialization/DeserializationWrapper.cs b/src/libraries/System.Text.Json/tests/Serialization/DeserializationWrapper.cs index 77eae2fbf85a7..508847edc3528 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/DeserializationWrapper.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/DeserializationWrapper.cs @@ -14,20 +14,27 @@ public abstract class DeserializationWrapper { private static readonly JsonSerializerOptions _optionsWithSmallBuffer = new JsonSerializerOptions { DefaultBufferSize = 1 }; - public static DeserializationWrapper StringTValueSerializer => new StringTValueSerializerWrapper(); - public static DeserializationWrapper StreamTValueSerializer => new StreamTValueSerializerWrapper(); + public static DeserializationWrapper StringDeserializer => new StringDeserializerWrapper(); + public static DeserializationWrapper StreamDeserializer => new StreamDeserializerWrapper(); protected internal abstract T Deserialize(string json, JsonSerializerOptions options = null); - private class StringTValueSerializerWrapper : DeserializationWrapper + protected internal abstract object Deserialize(string json, Type type, JsonSerializerOptions options = null); + + private class StringDeserializerWrapper : DeserializationWrapper { protected internal override T Deserialize(string json, JsonSerializerOptions options = null) { return JsonSerializer.Deserialize(json, options); } + + protected internal override object Deserialize(string json, Type type, JsonSerializerOptions options = null) + { + return JsonSerializer.Deserialize(json, type, options); + } } - private class StreamTValueSerializerWrapper : DeserializationWrapper + private class StreamDeserializerWrapper : DeserializationWrapper { protected internal override T Deserialize(string json, JsonSerializerOptions options = null) { @@ -44,6 +51,22 @@ protected internal override T Deserialize(string json, JsonSerializerOptions } }).GetAwaiter().GetResult(); } + + protected internal override object Deserialize(string json, Type type, JsonSerializerOptions options = null) + { + if (options == null) + { + options = _optionsWithSmallBuffer; + } + + return Task.Run(async () => + { + using (MemoryStream stream = new MemoryStream(Encoding.UTF8.GetBytes(json))) + { + return await JsonSerializer.DeserializeAsync(stream, type, options); + } + }).GetAwaiter().GetResult(); + } } } } diff --git a/src/libraries/System.Text.Json/tests/Serialization/TestClasses.Constructor.cs b/src/libraries/System.Text.Json/tests/Serialization/TestClasses.Constructor.cs index c0be977ac84a6..5358f7e61501b 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/TestClasses.Constructor.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/TestClasses.Constructor.cs @@ -10,6 +10,11 @@ namespace System.Text.Json.Serialization.Tests { + public interface ITestClassWithParameterizedCtor : ITestClass + { + void VerifyMinimal(); + } + public class PrivateParameterlessCtor { private PrivateParameterlessCtor() { } @@ -572,7 +577,7 @@ public struct Point_3D_Struct public Point_3D_Struct(int x, int y, int z = 50) => (X, Y, Z) = (x, y, z); } - public class Person_Class : ITestClass + public class Person_Class : ITestClassWithParameterizedCtor { public string FirstName { get; set; } public string LastName { get; set; } @@ -616,22 +621,32 @@ public Person_Class( ReadOnlySinglePublicParameterizedCtor = readOnlySinglePublicParameterizedCtor; } - public static readonly string s_json = - @"{ + public static string s_json => $"{{{s_partialJson1},{s_partialJson2}}}"; + + public static string s_json_flipped => $"{{{s_partialJson2},{s_partialJson1}}}"; + + public static string s_json_minimal => @"{""ReadOnlyPoint2D"":{""X"":1,""Y"":2}}"; + + private const string s_partialJson1 = + @" ""FirstName"":""John"", ""LastName"":""Doe"", ""EmailAddress"":""johndoe@live.com"", ""Id"":""f2c92fcc-459f-4287-90b6-a7cbd82aeb0e"", ""Age"":24, ""Point2D"":{""X"":1,""Y"":2}, - ""ReadOnlyPoint2D"":{""X"":1,""Y"":2}, + ""ReadOnlyPoint2D"":{""X"":1,""Y"":2} + "; + + private const string s_partialJson2 = + @" ""Point2DWithExtDataClass"":{""X"":1,""Y"":2,""b"":3}, ""ReadOnlyPoint2DWithExtDataClass"":{""X"":1,""Y"":2,""b"":3}, ""Point3DStruct"":{""X"":1,""Y"":2,""Z"":3}, ""ReadOnlyPoint3DStruct"":{""X"":1,""Y"":2,""Z"":3}, ""Point2DWithExtData"":{""X"":1,""Y"":2,""b"":3}, ""ReadOnlyPoint2DWithExtData"":{""X"":1,""Y"":2,""b"":3} - }"; + "; public static readonly byte[] s_data = Encoding.UTF8.GetBytes(s_json); @@ -693,6 +708,13 @@ public void Verify() Assert.Contains(@"""Y"":2", serialized); Assert.Contains(@"""b"":3", serialized); } + + public void VerifyMinimal() + { + string serialized = JsonSerializer.Serialize(ReadOnlyPoint2D); + Assert.Contains(@"""X"":1", serialized); + Assert.Contains(@"""Y"":2", serialized); + } } public struct Person_Struct : ITestClass @@ -895,7 +917,7 @@ public NullArgTester(Point_3D_Struct point3DStruct, ImmutableArray immutabl } } - public class ClassWithConstructor_SimpleAndComplexParameters : ITestClass + public class ClassWithConstructor_SimpleAndComplexParameters : ITestClassWithParameterizedCtor { public byte MyByte { get; } public sbyte MySByte { get; set; } @@ -986,8 +1008,11 @@ public ClassWithConstructor_SimpleAndComplexParameters( public static ClassWithConstructor_SimpleAndComplexParameters GetInstance() => JsonSerializer.Deserialize(s_json); - public static readonly string s_json = $"{{{s_partialJson1},{s_partialJson2}}}"; - public static readonly string s_json_flipped = $"{{{s_partialJson2},{s_partialJson1}}}"; + public static string s_json => $"{{{s_partialJson1},{s_partialJson2}}}"; + + public static string s_json_flipped => $"{{{s_partialJson2},{s_partialJson1}}}"; + + public static string s_json_minimal => @"{""MyDecimal"" : 3.3}"; private const string s_partialJson1 = @"""MyByte"" : 7," + @@ -1107,6 +1132,8 @@ public void Verify() Assert.Null(MyListOfNullString[0]); } + + public void VerifyMinimal() => Assert.Equal(3.3m, MyDecimal); } public class Parameterless_ClassWithPrimitives { @@ -1825,7 +1852,7 @@ public class CampaignSummaryViewModel public string Headline { get; set; } } - public class Parameterized_Class_With_ComplexTuple : ITestClass + public class Parameterized_Class_With_ComplexTuple : ITestClassWithParameterizedCtor { public Tuple< ClassWithConstructor_SimpleAndComplexParameters, @@ -1846,7 +1873,7 @@ public Parameterized_Class_With_ComplexTuple( ClassWithConstructor_SimpleAndComplexParameters, ClassWithConstructor_SimpleAndComplexParameters> myTuple) => MyTuple = myTuple; - private static readonly string s_inner_json = @" + private const string s_inner_json = @" { ""MyByte"": 7, ""MyChar"": ""a"", @@ -1911,19 +1938,44 @@ public Parameterized_Class_With_ComplexTuple( ""MyStringImmutablQueueT"": [ ""Hello"" ] }"; - public static readonly string s_json = + public static string s_json => + $@"{{ + ""MyTuple"": {{ + {s_partialJson1}, + {s_partialJson2} + }} + }}"; + + public static string s_json_flipped => + $@"{{ + ""MyTuple"": {{ + {s_partialJson2}, + {s_partialJson1} + }} + }}"; + + public static string s_json_minimal => $@"{{ ""MyTuple"": {{ - ""Item1"":{s_inner_json}, - ""Item2"":{s_inner_json}, - ""Item3"":{s_inner_json}, - ""Item4"":{s_inner_json}, - ""Item5"":{s_inner_json}, - ""Item6"":{s_inner_json}, - ""Item7"":{s_inner_json} + ""Item4"":{s_inner_json} }} }}"; + private static string s_partialJson1 => + $@" + ""Item1"":{s_inner_json}, + ""Item2"":{s_inner_json}, + ""Item3"":{s_inner_json}, + ""Item4"":{s_inner_json} + "; + + private static string s_partialJson2 => + $@" + ""Item5"":{s_inner_json}, + ""Item6"":{s_inner_json}, + ""Item7"":{s_inner_json} + "; + public static readonly byte[] s_data = Encoding.UTF8.GetBytes(s_json); public void Initialize() { } @@ -1938,6 +1990,11 @@ public void Verify() MyTuple.Item6.Verify(); MyTuple.Item7.Verify(); } + + public void VerifyMinimal() + { + MyTuple.Item4.Verify(); + } } public class Point_MembersHave_JsonPropertyName : ITestClass From 6507a9dfc774db7bef7718d5973f6dbd58ba9b02 Mon Sep 17 00:00:00 2001 From: Layomi Akinrinade Date: Mon, 30 Mar 2020 12:17:36 -0700 Subject: [PATCH 2/3] Address review feedback - move delegate assignment to start of deserialization --- ...ithParameterizedConstructorConverter.Large.cs | 16 +++++++++------- ...ithParameterizedConstructorConverter.Small.cs | 15 +++++++++------ .../Text/Json/Serialization/JsonClassInfo.cs | 7 ------- .../Text/Json/Serialization/JsonConverter.cs | 2 -- 4 files changed, 18 insertions(+), 22 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs index f62766a24abcc..221813d6f081f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs @@ -12,11 +12,6 @@ namespace System.Text.Json.Serialization.Converters /// internal sealed class LargeObjectWithParameterizedConstructorConverter : ObjectWithParameterizedConstructorConverter where T : notnull { - internal override void CreateConstructorDelegate(JsonClassInfo classInfo, JsonSerializerOptions options) - { - classInfo.CreateObjectWithParameterizedCtor = options.MemberAccessorStrategy.CreateParameterizedConstructor(ConstructorInfo)!; - } - protected override bool ReadAndCacheConstructorArgument(ref ReadStack state, ref Utf8JsonReader reader, JsonParameterInfo jsonParameterInfo) { bool success = jsonParameterInfo.ReadJson(ref state, ref reader, out object? arg0); @@ -49,8 +44,15 @@ protected override object CreateObject(ref ReadStackFrame frame) protected override void InitializeConstructorArgumentCaches(ref ReadStack state, JsonSerializerOptions options) { - object[] arguments = ArrayPool.Shared.Rent(state.Current.JsonClassInfo.ParameterCount); - foreach (JsonParameterInfo jsonParameterInfo in state.Current.JsonClassInfo.ParameterCache!.Values) + JsonClassInfo classInfo = state.Current.JsonClassInfo; + + if (classInfo.CreateObjectWithParameterizedCtor == null) + { + classInfo.CreateObjectWithParameterizedCtor = options.MemberAccessorStrategy.CreateParameterizedConstructor(ConstructorInfo)!; + } + + object[] arguments = ArrayPool.Shared.Rent(classInfo.ParameterCount); + foreach (JsonParameterInfo jsonParameterInfo in classInfo.ParameterCache!.Values) { if (jsonParameterInfo.ShouldDeserialize) { diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs index 70afd39f9c14a..eca390dc2c9d7 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs @@ -12,11 +12,6 @@ namespace System.Text.Json.Serialization.Converters /// internal sealed class SmallObjectWithParameterizedConstructorConverter : ObjectWithParameterizedConstructorConverter where T : notnull { - internal override void CreateConstructorDelegate(JsonClassInfo classInfo, JsonSerializerOptions options) - { - classInfo.CreateObjectWithParameterizedCtor = options.MemberAccessorStrategy.CreateParameterizedConstructor(ConstructorInfo)!; - } - protected override object CreateObject(ref ReadStackFrame frame) { var createObject = (JsonClassInfo.ParameterizedConstructorDelegate) @@ -72,9 +67,17 @@ protected override bool ReadAndCacheConstructorArgument(ref ReadStack state, ref protected override void InitializeConstructorArgumentCaches(ref ReadStack state, JsonSerializerOptions options) { + JsonClassInfo classInfo = state.Current.JsonClassInfo; + + if (classInfo.CreateObjectWithParameterizedCtor == null) + { + classInfo.CreateObjectWithParameterizedCtor = + options.MemberAccessorStrategy.CreateParameterizedConstructor(ConstructorInfo)!; + } + var arguments = new Arguments(); - foreach (JsonParameterInfo parameterInfo in state.Current.JsonClassInfo.ParameterCache!.Values) + foreach (JsonParameterInfo parameterInfo in classInfo.ParameterCache!.Values) { if (parameterInfo.ShouldDeserialize) { diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs index 48d220efc6296..8bfb451fcff45 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs @@ -161,13 +161,6 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) if (converter.ConstructorIsParameterized) { - // Create and cache the constructor delegate for this type. - // We depend on the first converter created for this type to perform the initialization. - // Subsequent instances of the converter will not perform this initialization since we will - // not go down this code path (as the type will already be in the JsonClassInfo cache). - // The converter is used because it has generic type support. - converter.CreateConstructorDelegate(this, options); - InitializeConstructorParameters(converter.ConstructorInfo); } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs index 98cabd6ef65ae..abc5ad446b9db 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs @@ -79,7 +79,5 @@ internal bool ShouldFlush(Utf8JsonWriter writer, ref WriteStack state) internal virtual bool ConstructorIsParameterized => false; internal ConstructorInfo ConstructorInfo { get; set; } = null!; - - internal virtual void CreateConstructorDelegate(JsonClassInfo classInfo, JsonSerializerOptions options) { } } } From 86a70da92421a9d6b71f1c4b8092df81c82a5f25 Mon Sep 17 00:00:00 2001 From: Layomi Akinrinade Date: Mon, 30 Mar 2020 13:21:51 -0700 Subject: [PATCH 3/3] Address review feedback - nullability --- ...ectWithParameterizedConstructorConverter.Large.cs | 12 ++++++------ ...ectWithParameterizedConstructorConverter.Small.cs | 6 +++--- .../ObjectWithParameterizedConstructorConverter.cs | 2 +- .../System/Text/Json/Serialization/JsonClassInfo.cs | 2 +- .../System/Text/Json/Serialization/JsonConverter.cs | 2 +- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs index 221813d6f081f..7444bd28ecb2c 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs @@ -18,7 +18,7 @@ protected override bool ReadAndCacheConstructorArgument(ref ReadStack state, ref if (success) { - ((object[])state.Current.CtorArgumentState!.Arguments!)[jsonParameterInfo.Position] = arg0!; + ((object[])state.Current.CtorArgumentState!.Arguments)[jsonParameterInfo.Position] = arg0!; } return success; @@ -26,17 +26,17 @@ protected override bool ReadAndCacheConstructorArgument(ref ReadStack state, ref protected override object CreateObject(ref ReadStackFrame frame) { - object[] arguments = (object[])frame.CtorArgumentState!.Arguments!; + object[] arguments = (object[])frame.CtorArgumentState!.Arguments; - var createObject = (JsonClassInfo.ParameterizedConstructorDelegate)frame.JsonClassInfo.CreateObjectWithParameterizedCtor!; + var createObject = (JsonClassInfo.ParameterizedConstructorDelegate?)frame.JsonClassInfo.CreateObjectWithParameterizedCtor; if (createObject == null) { // This means this constructor has more than 64 parameters. - ThrowHelper.ThrowNotSupportedException_ConstructorMaxOf64Parameters(ConstructorInfo, TypeToConvert); + ThrowHelper.ThrowNotSupportedException_ConstructorMaxOf64Parameters(ConstructorInfo!, TypeToConvert); } - object obj = createObject(arguments)!; + object obj = createObject(arguments); ArrayPool.Shared.Return(arguments, clearArray: true); return obj; @@ -48,7 +48,7 @@ protected override void InitializeConstructorArgumentCaches(ref ReadStack state, if (classInfo.CreateObjectWithParameterizedCtor == null) { - classInfo.CreateObjectWithParameterizedCtor = options.MemberAccessorStrategy.CreateParameterizedConstructor(ConstructorInfo)!; + classInfo.CreateObjectWithParameterizedCtor = options.MemberAccessorStrategy.CreateParameterizedConstructor(ConstructorInfo!); } object[] arguments = ArrayPool.Shared.Rent(classInfo.ParameterCount); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs index eca390dc2c9d7..97ed0414b0fc2 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs @@ -16,8 +16,8 @@ protected override object CreateObject(ref ReadStackFrame frame) { var createObject = (JsonClassInfo.ParameterizedConstructorDelegate) frame.JsonClassInfo.CreateObjectWithParameterizedCtor!; - var arguments = (Arguments)frame.CtorArgumentState!.Arguments!; - return createObject!(arguments.Arg0, arguments.Arg1, arguments.Arg2, arguments.Arg3)!; + var arguments = (Arguments)frame.CtorArgumentState!.Arguments; + return createObject!(arguments.Arg0, arguments.Arg1, arguments.Arg2, arguments.Arg3); } protected override bool ReadAndCacheConstructorArgument(ref ReadStack state, ref Utf8JsonReader reader, JsonParameterInfo jsonParameterInfo) @@ -72,7 +72,7 @@ protected override void InitializeConstructorArgumentCaches(ref ReadStack state, if (classInfo.CreateObjectWithParameterizedCtor == null) { classInfo.CreateObjectWithParameterizedCtor = - options.MemberAccessorStrategy.CreateParameterizedConstructor(ConstructorInfo)!; + options.MemberAccessorStrategy.CreateParameterizedConstructor(ConstructorInfo!); } var arguments = new Arguments(); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs index 40424126c1152..751361d55ef0f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs @@ -429,7 +429,7 @@ private void BeginRead(ref ReadStack state, ref Utf8JsonReader reader, JsonSeria if (state.Current.JsonClassInfo.ParameterCount != state.Current.JsonClassInfo.ParameterCache!.Count) { - ThrowHelper.ThrowInvalidOperationException_ConstructorParameterIncompleteBinding(ConstructorInfo, TypeToConvert); + ThrowHelper.ThrowInvalidOperationException_ConstructorParameterIncompleteBinding(ConstructorInfo!, TypeToConvert); } // Set current JsonPropertyInfo to null to avoid conflicts on push. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs index 8bfb451fcff45..362614918cdcd 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs @@ -161,7 +161,7 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) if (converter.ConstructorIsParameterized) { - InitializeConstructorParameters(converter.ConstructorInfo); + InitializeConstructorParameters(converter.ConstructorInfo!); } } break; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs index abc5ad446b9db..9b7a706678c36 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs @@ -78,6 +78,6 @@ internal bool ShouldFlush(Utf8JsonWriter writer, ref WriteStack state) // Whether a type (ClassType.Object) is deserialized using a parameterized constructor. internal virtual bool ConstructorIsParameterized => false; - internal ConstructorInfo ConstructorInfo { get; set; } = null!; + internal ConstructorInfo? ConstructorInfo { get; set; } } }