From 709762896d00cabf6d0939223d8ddd72babfd0ba Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Thu, 17 Sep 2020 15:03:01 -0500 Subject: [PATCH 1/2] Make nullable-related checks consistent and faster --- .../Value/NullableConverterFactory.cs | 2 +- .../Json/Serialization/GenericMethodHolder.cs | 2 +- .../Json/Serialization/JsonConverterOfT.cs | 2 +- .../Json/Serialization/JsonPropertyInfo.cs | 1 - .../Json/Serialization/JsonPropertyInfoOfT.cs | 5 ++-- .../JsonSerializerOptions.Converters.cs | 2 +- .../src/System/Text/Json/TypeExtensions.cs | 24 +++++++++---------- 7 files changed, 17 insertions(+), 21 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverterFactory.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverterFactory.cs index 19679337337b90..dfab566a636a8e 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverterFactory.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverterFactory.cs @@ -10,7 +10,7 @@ internal class NullableConverterFactory : JsonConverterFactory { public override bool CanConvert(Type typeToConvert) { - return Nullable.GetUnderlyingType(typeToConvert) != null; + return typeToConvert.IsNullableOfT(); } public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/GenericMethodHolder.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/GenericMethodHolder.cs index ef8e362e1a695b..d321ab2832f19e 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/GenericMethodHolder.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/GenericMethodHolder.cs @@ -26,7 +26,7 @@ public override bool IsDefaultValue(object value) { // For performance, we only want to call this method for non-nullable value types. // Nullable types should be checked againt 'null' before calling this method. - Debug.Assert(Nullable.GetUnderlyingType(value.GetType()) == null); + Debug.Assert(!value.GetType().CanBeNull()); return EqualityComparer.Default.Equals(default, (T)value); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs index 38bd9cb480f8b5..348c558994aa63 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs @@ -21,7 +21,7 @@ protected internal JsonConverter() // In the future, this will be check for !IsSealed (and excluding value types). CanBePolymorphic = TypeToConvert == JsonClassInfo.ObjectType; IsValueType = TypeToConvert.IsValueType; - CanBeNull = !IsValueType || Nullable.GetUnderlyingType(TypeToConvert) != null; + CanBeNull = !IsValueType || TypeToConvert.IsNullableOfT(); IsInternalConverter = GetType().Assembly == typeof(JsonConverter).Assembly; if (HandleNull) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs index f13201901f7b5f..4d7d9f40f2c816 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs @@ -12,7 +12,6 @@ namespace System.Text.Json internal abstract class JsonPropertyInfo { public static readonly JsonPropertyInfo s_missingProperty = GetPropertyPlaceholder(); - public static readonly Type s_NullableType = typeof(Nullable<>); private JsonClassInfo? _runtimeClassInfo; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs index 612db8c34967e4..114858d4e32ba5 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs @@ -105,8 +105,7 @@ public override void Initialize( } _converterIsExternalAndPolymorphic = !converter.IsInternalConverter && DeclaredPropertyType != converter.TypeToConvert; - _propertyTypeCanBeNull = !DeclaredPropertyType.IsValueType || - (DeclaredPropertyType.IsGenericType && DeclaredPropertyType.GetGenericTypeDefinition() == s_NullableType); + _propertyTypeCanBeNull = DeclaredPropertyType.CanBeNull(); _propertyTypeEqualsTypeToConvert = typeof(T) == DeclaredPropertyType; GetPolicies(ignoreCondition, parentTypeNumberHandling, defaultValueIsNull: _propertyTypeCanBeNull); @@ -285,7 +284,7 @@ public override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref U ThrowHelper.ThrowInvalidCastException_DeserializeUnableToAssignValue(typeOfValue, DeclaredPropertyType); } } - else if (DeclaredPropertyType.IsValueType && !DeclaredPropertyType.IsNullableValueType()) + else if (!_propertyTypeCanBeNull) { ThrowHelper.ThrowInvalidOperationException_DeserializeUnableToAssignNull(DeclaredPropertyType); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs index 8d0c4f1f60b691..87049b4ccee44a 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs @@ -185,7 +185,7 @@ internal JsonConverter DetermineConverter(Type? parentClassType, Type runtimePro // We also throw to avoid passing an invalid argument to setters for nullable struct properties, // which would cause an InvalidProgramException when the generated IL is invoked. // This is not an issue of the converter is wrapped in NullableConverter. - if (runtimePropertyType.IsNullableType() && !converter.TypeToConvert.IsNullableType()) + if (runtimePropertyType.CanBeNull() && !converter.TypeToConvert.CanBeNull()) { ThrowHelper.ThrowInvalidOperationException_ConverterCanConvertNullableRedundant(runtimePropertyType, converter); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs b/src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs index 8abbf6d6c7b2fd..04ae9ac8cb23b6 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs @@ -7,31 +7,29 @@ namespace System.Text.Json { internal static class TypeExtensions { + private static readonly Type s_NullableType = typeof(Nullable<>); + /// /// Returns when the given type is of type . /// - public static bool IsNullableValueType(this Type type) - { - return Nullable.GetUnderlyingType(type) != null; - } + public static bool IsNullableOfT(this Type type) => + type.IsGenericType && type.GetGenericTypeDefinition() == s_NullableType; /// /// Returns when the given type is either a reference type or of type . /// - public static bool IsNullableType(this Type type) - { - return !type.IsValueType || IsNullableValueType(type); - } + /// This calls which is slow. If knowledge already exists + /// that the type is a value type, call instead. + public static bool CanBeNull(this Type type) => + !type.IsValueType || (type.IsGenericType && type.GetGenericTypeDefinition() == s_NullableType); /// - /// Returns when the given type is assignable from . + /// Returns when the given type is assignable from including support + /// when is by using the {T} generic parameter for . /// - /// - /// Other than also returns when is of type where : and is of type . - /// public static bool IsAssignableFromInternal(this Type type, Type from) { - if (IsNullableValueType(from) && type.IsInterface) + if (IsNullableOfT(from) && type.IsInterface) { return type.IsAssignableFrom(from.GetGenericArguments()[0]); } From 6f04bb90130adcf5d87acaf387e673fa878f5a5a Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Tue, 22 Sep 2020 11:00:51 -0500 Subject: [PATCH 2/2] Review changes --- .../System.Text.Json/src/System.Text.Json.csproj | 4 ++-- .../src/System/{Text/Json => }/TypeExtensions.cs | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) rename src/libraries/System.Text.Json/src/System/{Text/Json => }/TypeExtensions.cs (83%) diff --git a/src/libraries/System.Text.Json/src/System.Text.Json.csproj b/src/libraries/System.Text.Json/src/System.Text.Json.csproj index 7542db83554cd6..f1ddddbebbf050 100644 --- a/src/libraries/System.Text.Json/src/System.Text.Json.csproj +++ b/src/libraries/System.Text.Json/src/System.Text.Json.csproj @@ -1,4 +1,4 @@ - + true $(NetCoreAppCurrent);netstandard2.0;netcoreapp3.0;net461 @@ -182,7 +182,6 @@ - @@ -217,6 +216,7 @@ + diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs b/src/libraries/System.Text.Json/src/System/TypeExtensions.cs similarity index 83% rename from src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs rename to src/libraries/System.Text.Json/src/System/TypeExtensions.cs index 04ae9ac8cb23b6..1e8352761bd456 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs +++ b/src/libraries/System.Text.Json/src/System/TypeExtensions.cs @@ -1,27 +1,29 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System; +using System.Runtime.CompilerServices; namespace System.Text.Json { internal static class TypeExtensions { - private static readonly Type s_NullableType = typeof(Nullable<>); + private static readonly Type s_nullableType = typeof(Nullable<>); /// /// Returns when the given type is of type . /// + [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool IsNullableOfT(this Type type) => - type.IsGenericType && type.GetGenericTypeDefinition() == s_NullableType; + type.IsGenericType && type.GetGenericTypeDefinition() == s_nullableType; /// /// Returns when the given type is either a reference type or of type . /// /// This calls which is slow. If knowledge already exists /// that the type is a value type, call instead. + [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool CanBeNull(this Type type) => - !type.IsValueType || (type.IsGenericType && type.GetGenericTypeDefinition() == s_NullableType); + !type.IsValueType || type.IsNullableOfT(); /// /// Returns when the given type is assignable from including support