From f65b7269f76ba5f7a70054f267ef520cadbeeb9a Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Mon, 9 Sep 2019 14:10:49 -0500 Subject: [PATCH 1/3] Improve perf of property name lookup --- .../Text/Json/Serialization/JsonClassInfo.cs | 189 ++++++++++-------- .../JsonSerializer.Read.HandlePropertyName.cs | 7 +- .../tests/Serialization/PropertyNameTests.cs | 82 ++++++++ 3 files changed, 196 insertions(+), 82 deletions(-) diff --git a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs index 2fa525bbf675..eb83f661e237 100644 --- a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs +++ b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs @@ -16,7 +16,7 @@ namespace System.Text.Json internal sealed partial class JsonClassInfo { // The length of the property name embedded in the key (in bytes). - private const int PropertyNameKeyLength = 6; + private const int PropertyNameKeyLength = 7; // The limit to how many property names from the JSON are cached in _propertyRefsSorted before using PropertyCache. private const int PropertyNameCountCacheThreshold = 64; @@ -259,16 +259,14 @@ private JsonPropertyInfo GetPropertyWithUniqueAttribute(Type attributeType, Dict public JsonPropertyInfo GetProperty(ReadOnlySpan propertyName, ref ReadStackFrame frame) { - JsonPropertyInfo info = null; - // Keep a local copy of the cache in case it changes by another thread. PropertyRef[] localPropertyRefsSorted = _propertyRefsSorted; + ulong key = GetKey(propertyName); + // If there is an existing cache, then use it. if (localPropertyRefsSorted != null) { - ulong key = GetKey(propertyName); - // Start with the current property index, and then go forwards\backwards. int propertyIndex = frame.PropertyIndex; @@ -276,66 +274,106 @@ public JsonPropertyInfo GetProperty(ReadOnlySpan propertyName, ref ReadSta int iForward = Math.Min(propertyIndex, count); int iBackward = iForward - 1; - while (iForward < count || iBackward >= 0) + for (;;) { if (iForward < count) { - if (TryIsPropertyRefEqual(localPropertyRefsSorted[iForward], propertyName, key, ref info)) + PropertyRef propertyRef = localPropertyRefsSorted[iForward]; + if (key == propertyRef.Key) { - return info; + if (propertyName.Length <= PropertyNameKeyLength || + // We compare the whole name, although we could skip the first 7 bytes (but it's not any faster) + propertyName.SequenceEqual(propertyRef.Info.Name)) + { + return propertyRef.Info; + } } ++iForward; - } - if (iBackward >= 0) + if (iBackward >= 0) + { + propertyRef = localPropertyRefsSorted[iBackward]; + // This path is the same code as above; copied here instead of creating a method since this is a hot path. + if (key == propertyRef.Key) + { + if (propertyName.Length <= PropertyNameKeyLength || + propertyName.SequenceEqual(propertyRef.Info.Name)) + { + return propertyRef.Info; + } + } + + --iBackward; + } + } + else if (iBackward >= 0) { - if (TryIsPropertyRefEqual(localPropertyRefsSorted[iBackward], propertyName, key, ref info)) + // This path is the same code as above; copied here instead of creating a method since this is a hot path. + PropertyRef propertyRef = localPropertyRefsSorted[iBackward]; + if (key == propertyRef.Key) { - return info; + if (propertyName.Length <= PropertyNameKeyLength || + propertyName.SequenceEqual(propertyRef.Info.Name)) + { + return propertyRef.Info; + } } + --iBackward; } + else + { + // Property was not found. + break; + } } } // No cached item was found. Try the main list which has all of the properties. string stringPropertyName = JsonHelpers.Utf8GetString(propertyName); - if (PropertyCache.TryGetValue(stringPropertyName, out info)) + if (!PropertyCache.TryGetValue(stringPropertyName, out JsonPropertyInfo info)) { - // Check if we should add this to the cache. - // Only cache up to a threshold length and then just use the dictionary when an item is not found in the cache. - int count; - if (localPropertyRefsSorted != null) - { - count = localPropertyRefsSorted.Length; - } - else + info = JsonPropertyInfo.s_missingProperty; + } + + // Three code paths to get here: + // 1) info == s_missingProperty. Property not found. + // 2) key == info.PropertyNameKey. Exact match found. + // 3) key != info.PropertyNameKey. Match found due to case insensitivity. + Debug.Assert(info == null || key == info.PropertyNameKey || Options.PropertyNameCaseInsensitive); + + // Check if we should add this to the cache. + // Only cache up to a threshold length and then just use the dictionary when an item is not found in the cache. + int cacheCount; + if (localPropertyRefsSorted != null) + { + cacheCount = localPropertyRefsSorted.Length; + } + else + { + cacheCount = 0; + } + + // Do a quick check for the stable (after warm-up) case. + if (cacheCount < PropertyNameCountCacheThreshold) + { + // Do a slower check for the warm-up case. + if (frame.PropertyRefCache != null) { - count = 0; + cacheCount += frame.PropertyRefCache.Count; } - // Do a quick check for the stable (after warm-up) case. - if (count < PropertyNameCountCacheThreshold) + // Check again to append the cache up to the threshold. + if (cacheCount < PropertyNameCountCacheThreshold) { - // Do a slower check for the warm-up case. - if (frame.PropertyRefCache != null) + if (frame.PropertyRefCache == null) { - count += frame.PropertyRefCache.Count; + frame.PropertyRefCache = new List(); } - // Check again to append the cache up to the threshold. - if (count < PropertyNameCountCacheThreshold) - { - if (frame.PropertyRefCache == null) - { - frame.PropertyRefCache = new List(); - } - - ulong key = info.PropertyNameKey; - PropertyRef propertyRef = new PropertyRef(key, info); - frame.PropertyRefCache.Add(propertyRef); - } + PropertyRef propertyRef = new PropertyRef(key, info); + frame.PropertyRefCache.Add(propertyRef); } } @@ -360,60 +398,52 @@ private Dictionary CreatePropertyCache(int capacity) public JsonPropertyInfo PolicyProperty { get; private set; } - private static bool TryIsPropertyRefEqual(in PropertyRef propertyRef, ReadOnlySpan propertyName, ulong key, ref JsonPropertyInfo info) - { - if (key == propertyRef.Key) - { - if (propertyName.Length <= PropertyNameKeyLength || - // We compare the whole name, although we could skip the first 6 bytes (but it's likely not any faster) - propertyName.SequenceEqual(propertyRef.Info.Name)) - { - info = propertyRef.Info; - return true; - } - } - - return false; - } - - private static bool IsPropertyRefEqual(ref PropertyRef propertyRef, PropertyRef other) - { - if (propertyRef.Key == other.Key) - { - if (propertyRef.Info.Name.Length <= PropertyNameKeyLength || - propertyRef.Info.Name.AsSpan().SequenceEqual(other.Info.Name.AsSpan())) - { - return true; - } - } - - return false; - } - + /// + /// Get a key from the property name. + /// If property name length is 7 or less then this key can be used for equality since + /// the first 7 bytes are encoded along with the length. + /// public static ulong GetKey(ReadOnlySpan propertyName) { ulong key; int length = propertyName.Length; - // Embed the propertyName in the first 6 bytes of the key. - if (length > 3) + // Embed the propertyName in the first 7 bytes of the key. + if (length > 7) + { + key = MemoryMarshal.Read(propertyName); + + // Clear the last byte so it can hold the length. + // Be consistent by using shift operators instead of & to avoid endianness issues. + key = key << 0x8 >> 0x8; + } + else if (length > 3) { key = MemoryMarshal.Read(propertyName); - if (length > 4) + + if (length == 7) { - key |= (ulong)propertyName[4] << 32; + key |= (ulong)propertyName[4] << 0x20 + | (ulong)propertyName[5] << 0x28 + | (ulong)propertyName[6] << 0x30; } - if (length > 5) + else if (length == 6) { - key |= (ulong)propertyName[5] << 40; + key |= (ulong)propertyName[4] << 0x20 + | (ulong)propertyName[5] << 0x28; + } + else if (length == 5) + { + key |= (ulong)propertyName[4] << 0x20; } } else if (length > 1) { key = MemoryMarshal.Read(propertyName); - if (length > 2) + + if (length == 3) { - key |= (ulong)propertyName[2] << 16; + key |= (ulong)propertyName[2] << 0x10; } } else if (length == 1) @@ -426,8 +456,9 @@ public static ulong GetKey(ReadOnlySpan propertyName) key = 0; } - // Embed the propertyName length in the last two bytes. - key |= (ulong)propertyName.Length << 48; + // Embed the length in the last byte. + key |= (ulong)Math.Min(propertyName.Length, 0xFF) << 0x38; + return key; } diff --git a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs index ced83b472836..3f42d0288a99 100644 --- a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs +++ b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs @@ -52,7 +52,7 @@ private static void HandlePropertyName( } JsonPropertyInfo jsonPropertyInfo = state.Current.JsonClassInfo.GetProperty(propertyName, ref state.Current); - if (jsonPropertyInfo == null) + if (jsonPropertyInfo == JsonPropertyInfo.s_missingProperty) { JsonPropertyInfo dataExtProperty = state.Current.JsonClassInfo.DataExtensionProperty; if (dataExtProperty == null) @@ -94,9 +94,10 @@ private static void HandlePropertyName( state.Current.JsonPropertyInfo.JsonPropertyName = propertyNameArray; } } - - state.Current.PropertyIndex++; } + + // Increment the PropertyIndex so JsonClassInfo.GetProperty() starts with the next property. + state.Current.PropertyIndex++; } } diff --git a/src/System.Text.Json/tests/Serialization/PropertyNameTests.cs b/src/System.Text.Json/tests/Serialization/PropertyNameTests.cs index e039ffdd7930..462c1512fc16 100644 --- a/src/System.Text.Json/tests/Serialization/PropertyNameTests.cs +++ b/src/System.Text.Json/tests/Serialization/PropertyNameTests.cs @@ -293,6 +293,87 @@ public static void ExtensionDataDictionarySerialize_DoesNotHonor() Assert.False(obj.MyOverflow.ContainsKey("key1")); Assert.Equal(1, obj.MyOverflow["Key1"].GetInt32()); } + + private class ClassWithPropertyNamePermutations + { + public int a { get; set; } + public int aa { get; set; } + public int aaa { get; set; } + public int aaaa { get; set; } + public int aaaaa { get; set; } + public int aaaaaa { get; set; } + + // 7 characters - caching code only keys up to 7. + public int aaaaaaa { get; set; } + public int aaaaaab { get; set; } + + // 8 characters. + public int aaaaaaaa { get; set; } + public int aaaaaaab { get; set; } + + // 9 characters - caching code doesn't differentiate past this. + public int aaaaaaaaa { get; set; } + public int aaaaaaaab { get; set; } + } + + [Fact] + public static void CachingKeys() + { + ClassWithPropertyNamePermutations obj = new ClassWithPropertyNamePermutations + { + a = 1, + aa = 2, + aaa = 3, + aaaa = 4, + aaaaa = 5, + aaaaaa = 6, + aaaaaaa = 7, + aaaaaab = 7, + aaaaaaaa = 8, + aaaaaaab = 8, + aaaaaaaaa = 9, + aaaaaaaab = 9, + }; + + string json = JsonSerializer.Serialize(obj); + obj = JsonSerializer.Deserialize(json); + + Assert.Equal(130, json.Length); + + Assert.Equal(1, obj.a); + Assert.Equal(2, obj.aa); + Assert.Equal(3, obj.aaa); + Assert.Equal(4, obj.aaaa); + Assert.Equal(5, obj.aaaaa); + Assert.Equal(6, obj.aaaaaa); + Assert.Equal(7, obj.aaaaaaa); + Assert.Equal(7, obj.aaaaaab); + Assert.Equal(8, obj.aaaaaaaa); + Assert.Equal(8, obj.aaaaaaab); + Assert.Equal(9, obj.aaaaaaaaa); + Assert.Equal(9, obj.aaaaaaaab); + } + + [Theory] + [InlineData(0x1)] + [InlineData(0x10)] + [InlineData(0x100)] + [InlineData(0x1000)] + [InlineData(0x10000)] + public static void LongPropertyNames(int propertyLength) + { + // Although the CLR may limit member length to 1023, the serializer doesn't have a hard limit. + + string val = new string('v', propertyLength); + string json = @"{""" + val + @""":1}"; + + EmptyClassWithExtensionProperty obj = JsonSerializer.Deserialize(json); + + Assert.True(obj.MyOverflow.ContainsKey(val)); + + json = JsonSerializer.Serialize(obj); + Assert.Contains(val, json); + } } public class OverridePropertyNameDesignTime_TestClass @@ -359,3 +440,4 @@ public class EmptyClassWithExtensionProperty public IDictionary MyOverflow { get; set; } } } + From d912a9121d452343288b58ff58345aec329ea15a Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Wed, 11 Sep 2019 15:29:47 -0500 Subject: [PATCH 2/3] Refactor GetKey(), use AggressiveInlining and improve tests --- .../Text/Json/Serialization/JsonClassInfo.cs | 113 ++++++++++++------ .../JsonSerializer.Read.HandlePropertyName.cs | 3 + .../JsonSerializer.Read.HandleValue.cs | 11 +- .../tests/Serialization/PropertyNameTests.cs | 79 +++++++++--- 4 files changed, 145 insertions(+), 61 deletions(-) diff --git a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs index eb83f661e237..3d618f0344ac 100644 --- a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs +++ b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Reflection; +using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Text.Json.Serialization; using System.Text.Json.Serialization.Converters; @@ -16,6 +17,8 @@ namespace System.Text.Json internal sealed partial class JsonClassInfo { // The length of the property name embedded in the key (in bytes). + // The key is a ulong (8 bytes) containing the first 7 bytes of the property name + // followed by a byte representing the length. private const int PropertyNameKeyLength = 7; // The limit to how many property names from the JSON are cached in _propertyRefsSorted before using PropertyCache. @@ -257,8 +260,29 @@ private JsonPropertyInfo GetPropertyWithUniqueAttribute(Type attributeType, Dict return property; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static bool TryIsPropertyRefEqual(in PropertyRef propertyRef, ReadOnlySpan propertyName, ulong key, ref JsonPropertyInfo info) + { + if (key == propertyRef.Key) + { + // We compare the whole name, although we could skip the first 7 bytes (but it's not any faster) + if (propertyName.Length <= PropertyNameKeyLength || + propertyName.SequenceEqual(propertyRef.Info.Name)) + { + info = propertyRef.Info; + return true; + } + } + + return false; + } + + // AggressiveInlining used although a large method it is only called from one location and is on a hot path. + [MethodImpl(MethodImplOptions.AggressiveInlining)] public JsonPropertyInfo GetProperty(ReadOnlySpan propertyName, ref ReadStackFrame frame) { + JsonPropertyInfo info = null; + // Keep a local copy of the cache in case it changes by another thread. PropertyRef[] localPropertyRefsSorted = _propertyRefsSorted; @@ -279,28 +303,19 @@ public JsonPropertyInfo GetProperty(ReadOnlySpan propertyName, ref ReadSta if (iForward < count) { PropertyRef propertyRef = localPropertyRefsSorted[iForward]; - if (key == propertyRef.Key) + if (TryIsPropertyRefEqual(propertyRef, propertyName, key, ref info)) { - if (propertyName.Length <= PropertyNameKeyLength || - // We compare the whole name, although we could skip the first 7 bytes (but it's not any faster) - propertyName.SequenceEqual(propertyRef.Info.Name)) - { - return propertyRef.Info; - } + return info; } + ++iForward; if (iBackward >= 0) { propertyRef = localPropertyRefsSorted[iBackward]; - // This path is the same code as above; copied here instead of creating a method since this is a hot path. - if (key == propertyRef.Key) + if (TryIsPropertyRefEqual(propertyRef, propertyName, key, ref info)) { - if (propertyName.Length <= PropertyNameKeyLength || - propertyName.SequenceEqual(propertyRef.Info.Name)) - { - return propertyRef.Info; - } + return info; } --iBackward; @@ -308,15 +323,10 @@ public JsonPropertyInfo GetProperty(ReadOnlySpan propertyName, ref ReadSta } else if (iBackward >= 0) { - // This path is the same code as above; copied here instead of creating a method since this is a hot path. PropertyRef propertyRef = localPropertyRefsSorted[iBackward]; - if (key == propertyRef.Key) + if (TryIsPropertyRefEqual(propertyRef, propertyName, key, ref info)) { - if (propertyName.Length <= PropertyNameKeyLength || - propertyName.SequenceEqual(propertyRef.Info.Name)) - { - return propertyRef.Info; - } + return info; } --iBackward; @@ -332,16 +342,18 @@ public JsonPropertyInfo GetProperty(ReadOnlySpan propertyName, ref ReadSta // No cached item was found. Try the main list which has all of the properties. string stringPropertyName = JsonHelpers.Utf8GetString(propertyName); - if (!PropertyCache.TryGetValue(stringPropertyName, out JsonPropertyInfo info)) + if (!PropertyCache.TryGetValue(stringPropertyName, out info)) { info = JsonPropertyInfo.s_missingProperty; } + Debug.Assert(info != null); + // Three code paths to get here: // 1) info == s_missingProperty. Property not found. // 2) key == info.PropertyNameKey. Exact match found. // 3) key != info.PropertyNameKey. Match found due to case insensitivity. - Debug.Assert(info == null || key == info.PropertyNameKey || Options.PropertyNameCaseInsensitive); + Debug.Assert(info == JsonPropertyInfo.s_missingProperty || key == info.PropertyNameKey || Options.PropertyNameCaseInsensitive); // Check if we should add this to the cache. // Only cache up to a threshold length and then just use the dictionary when an item is not found in the cache. @@ -400,22 +412,25 @@ private Dictionary CreatePropertyCache(int capacity) /// /// Get a key from the property name. - /// If property name length is 7 or less then this key can be used for equality since - /// the first 7 bytes are encoded along with the length. + /// The key consists of the first 7 bytes of the property name and then the length. /// public static ulong GetKey(ReadOnlySpan propertyName) { + const int BitsInByte = 8; ulong key; int length = propertyName.Length; - // Embed the propertyName in the first 7 bytes of the key. if (length > 7) { key = MemoryMarshal.Read(propertyName); - // Clear the last byte so it can hold the length. - // Be consistent by using shift operators instead of & to avoid endianness issues. - key = key << 0x8 >> 0x8; + // Clear the high byte so it can hold the length. + key &= 0x00FFFFFFFFFFFFFF; + + // Use a length of 8. Any length > 7 (PropertyNameKeyLength) would work since + // the comparison logic tests for equality against the full contents instead of just + // the key if the property name is 8 or more characters. + key |= (ulong) 8 << (7 * BitsInByte); } else if (length > 3) { @@ -423,18 +438,25 @@ public static ulong GetKey(ReadOnlySpan propertyName) if (length == 7) { - key |= (ulong)propertyName[4] << 0x20 - | (ulong)propertyName[5] << 0x28 - | (ulong)propertyName[6] << 0x30; + key |= (ulong) propertyName[4] << (4 * BitsInByte) + | (ulong) propertyName[5] << (5 * BitsInByte) + | (ulong) propertyName[6] << (6 * BitsInByte) + | (ulong) 7 << (7 * BitsInByte); } else if (length == 6) { - key |= (ulong)propertyName[4] << 0x20 - | (ulong)propertyName[5] << 0x28; + key |= (ulong) propertyName[4] << (4 * BitsInByte) + | (ulong) propertyName[5] << (5 * BitsInByte) + | (ulong) 6 << (7 * BitsInByte); } else if (length == 5) { - key |= (ulong)propertyName[4] << 0x20; + key |= (ulong) propertyName[4] << (4 * BitsInByte) + | (ulong) 5 << (7 * BitsInByte); + } + else + { + key |= (ulong) 4 << (7 * BitsInByte); } } else if (length > 1) @@ -443,12 +465,18 @@ public static ulong GetKey(ReadOnlySpan propertyName) if (length == 3) { - key |= (ulong)propertyName[2] << 0x10; + key |= (ulong) propertyName[2] << (2 * BitsInByte) + | (ulong) 3 << (7 * BitsInByte); + } + else + { + key |= (ulong) 2 << (7 * BitsInByte); } } else if (length == 1) { - key = propertyName[0]; + key = propertyName[0] + | (ulong) 1 << (7 * BitsInByte); } else { @@ -456,8 +484,15 @@ public static ulong GetKey(ReadOnlySpan propertyName) key = 0; } - // Embed the length in the last byte. - key |= (ulong)Math.Min(propertyName.Length, 0xFF) << 0x38; + // Verify key contains the embedded bytes as expected. + Debug.Assert( + length < 1 || propertyName[0] == (key & ((ulong)0xFF << 8 * 0)) >> 8 * 0 && + length < 2 || propertyName[1] == (key & ((ulong)0xFF << 8 * 1)) >> 8 * 1 && + length < 3 || propertyName[2] == (key & ((ulong)0xFF << 8 * 2)) >> 8 * 2 && + length < 4 || propertyName[3] == (key & ((ulong)0xFF << 8 * 3)) >> 8 * 3 && + length < 5 || propertyName[4] == (key & ((ulong)0xFF << 8 * 4)) >> 8 * 4 && + length < 6 || propertyName[5] == (key & ((ulong)0xFF << 8 * 5)) >> 8 * 5 && + length < 7 || propertyName[6] == (key & ((ulong)0xFF << 8 * 6)) >> 8 * 6); return key; } diff --git a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs index 3f42d0288a99..92cf2b80ef17 100644 --- a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs +++ b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs @@ -5,11 +5,14 @@ using System.Buffers; using System.Collections; using System.Diagnostics; +using System.Runtime.CompilerServices; namespace System.Text.Json { public static partial class JsonSerializer { + // AggressiveInlining used although a large method it is only called from one locations and is on a hot path. + [MethodImpl(MethodImplOptions.AggressiveInlining)] private static void HandlePropertyName( JsonSerializerOptions options, ref Utf8JsonReader reader, diff --git a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleValue.cs b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleValue.cs index c674348de9da..fb15f4e23662 100644 --- a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleValue.cs +++ b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleValue.cs @@ -2,15 +2,19 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Runtime.CompilerServices; + namespace System.Text.Json { public static partial class JsonSerializer { - private static bool HandleValue(JsonTokenType tokenType, JsonSerializerOptions options, ref Utf8JsonReader reader, ref ReadStack state) + // AggressiveInlining used although a large method it is only called from two locations and is on a hot path. + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static void HandleValue(JsonTokenType tokenType, JsonSerializerOptions options, ref Utf8JsonReader reader, ref ReadStack state) { if (state.Current.SkipProperty) { - return false; + return; } JsonPropertyInfo jsonPropertyInfo = state.Current.JsonPropertyInfo; @@ -23,10 +27,7 @@ private static bool HandleValue(JsonTokenType tokenType, JsonSerializerOptions o jsonPropertyInfo = state.Current.JsonClassInfo.CreatePolymorphicProperty(jsonPropertyInfo, typeof(object), options); } - bool lastCall = (!state.Current.IsProcessingEnumerableOrDictionary && state.Current.ReturnValue == null); - jsonPropertyInfo.Read(tokenType, ref state, ref reader); - return lastCall; } } } diff --git a/src/System.Text.Json/tests/Serialization/PropertyNameTests.cs b/src/System.Text.Json/tests/Serialization/PropertyNameTests.cs index 462c1512fc16..3781fd6e6feb 100644 --- a/src/System.Text.Json/tests/Serialization/PropertyNameTests.cs +++ b/src/System.Text.Json/tests/Serialization/PropertyNameTests.cs @@ -314,12 +314,52 @@ private class ClassWithPropertyNamePermutations // 9 characters - caching code doesn't differentiate past this. public int aaaaaaaaa { get; set; } public int aaaaaaaab { get; set; } + + public int \u0467 { get; set; } + public int \u0467\u0467 { get; set; } + public int \u0467\u0467a { get; set; } + public int \u0467\u0467b { get; set; } + public int \u0467\u0467\u0467 { get; set; } + public int \u0467\u0467\u0467a { get; set; } + public int \u0467\u0467\u0467b { get; set; } + public int \u0467\u0467\u0467\u0467 { get; set; } + public int \u0467\u0467\u0467\u0467a { get; set; } + public int \u0467\u0467\u0467\u0467b { get; set; } } [Fact] public static void CachingKeys() { - ClassWithPropertyNamePermutations obj = new ClassWithPropertyNamePermutations + ClassWithPropertyNamePermutations obj; + + void Verify() + { + Assert.Equal(1, obj.a); + Assert.Equal(2, obj.aa); + Assert.Equal(3, obj.aaa); + Assert.Equal(4, obj.aaaa); + Assert.Equal(5, obj.aaaaa); + Assert.Equal(6, obj.aaaaaa); + Assert.Equal(7, obj.aaaaaaa); + Assert.Equal(7, obj.aaaaaab); + Assert.Equal(8, obj.aaaaaaaa); + Assert.Equal(8, obj.aaaaaaab); + Assert.Equal(9, obj.aaaaaaaaa); + Assert.Equal(9, obj.aaaaaaaab); + + Assert.Equal(2, obj.\u0467); + Assert.Equal(4, obj.\u0467\u0467); + Assert.Equal(5, obj.\u0467\u0467a); + Assert.Equal(5, obj.\u0467\u0467b); + Assert.Equal(6, obj.\u0467\u0467\u0467); + Assert.Equal(7, obj.\u0467\u0467\u0467a); + Assert.Equal(7, obj.\u0467\u0467\u0467b); + Assert.Equal(8, obj.\u0467\u0467\u0467\u0467); + Assert.Equal(9, obj.\u0467\u0467\u0467\u0467a); + Assert.Equal(9, obj.\u0467\u0467\u0467\u0467b); + } + + obj = new ClassWithPropertyNamePermutations { a = 1, aa = 2, @@ -333,25 +373,30 @@ public static void CachingKeys() aaaaaaab = 8, aaaaaaaaa = 9, aaaaaaaab = 9, + \u0467 = 2, + \u0467\u0467 = 4, + \u0467\u0467a = 5, + \u0467\u0467b = 5, + \u0467\u0467\u0467 = 6, + \u0467\u0467\u0467a = 7, + \u0467\u0467\u0467b = 7, + \u0467\u0467\u0467\u0467 = 8, + \u0467\u0467\u0467\u0467a = 9, + \u0467\u0467\u0467\u0467b = 9, }; + // Verify baseline. + Verify(); + string json = JsonSerializer.Serialize(obj); + + // Verify the length is consistent with a verified value. + Assert.Equal(354, json.Length); + obj = JsonSerializer.Deserialize(json); - Assert.Equal(130, json.Length); - - Assert.Equal(1, obj.a); - Assert.Equal(2, obj.aa); - Assert.Equal(3, obj.aaa); - Assert.Equal(4, obj.aaaa); - Assert.Equal(5, obj.aaaaa); - Assert.Equal(6, obj.aaaaaa); - Assert.Equal(7, obj.aaaaaaa); - Assert.Equal(7, obj.aaaaaab); - Assert.Equal(8, obj.aaaaaaaa); - Assert.Equal(8, obj.aaaaaaab); - Assert.Equal(9, obj.aaaaaaaaa); - Assert.Equal(9, obj.aaaaaaaab); + // Verify round-tripped object. + Verify(); } [Theory] @@ -371,8 +416,8 @@ public static void LongPropertyNames(int propertyLength) Assert.True(obj.MyOverflow.ContainsKey(val)); - json = JsonSerializer.Serialize(obj); - Assert.Contains(val, json); + string jsonRoundTripped = JsonSerializer.Serialize(obj); + Assert.Equal(json, jsonRoundTripped); } } From 491b466b312ae1c6359b73038abcb2cbd8169895 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Thu, 12 Sep 2019 13:23:47 -0500 Subject: [PATCH 3/3] Misc feedback --- .../Text/Json/Serialization/JsonClassInfo.cs | 75 +++++++++---------- .../tests/Serialization/PropertyNameTests.cs | 32 +++++--- 2 files changed, 55 insertions(+), 52 deletions(-) diff --git a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs index 3d618f0344ac..ba52359ba384 100644 --- a/src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs +++ b/src/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs @@ -260,23 +260,6 @@ private JsonPropertyInfo GetPropertyWithUniqueAttribute(Type attributeType, Dict return property; } - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static bool TryIsPropertyRefEqual(in PropertyRef propertyRef, ReadOnlySpan propertyName, ulong key, ref JsonPropertyInfo info) - { - if (key == propertyRef.Key) - { - // We compare the whole name, although we could skip the first 7 bytes (but it's not any faster) - if (propertyName.Length <= PropertyNameKeyLength || - propertyName.SequenceEqual(propertyRef.Info.Name)) - { - info = propertyRef.Info; - return true; - } - } - - return false; - } - // AggressiveInlining used although a large method it is only called from one location and is on a hot path. [MethodImpl(MethodImplOptions.AggressiveInlining)] public JsonPropertyInfo GetProperty(ReadOnlySpan propertyName, ref ReadStackFrame frame) @@ -298,7 +281,7 @@ public JsonPropertyInfo GetProperty(ReadOnlySpan propertyName, ref ReadSta int iForward = Math.Min(propertyIndex, count); int iBackward = iForward - 1; - for (;;) + while (true) { if (iForward < count) { @@ -357,15 +340,11 @@ public JsonPropertyInfo GetProperty(ReadOnlySpan propertyName, ref ReadSta // Check if we should add this to the cache. // Only cache up to a threshold length and then just use the dictionary when an item is not found in the cache. - int cacheCount; + int cacheCount = 0; if (localPropertyRefsSorted != null) { cacheCount = localPropertyRefsSorted.Length; } - else - { - cacheCount = 0; - } // Do a quick check for the stable (after warm-up) case. if (cacheCount < PropertyNameCountCacheThreshold) @@ -410,6 +389,23 @@ private Dictionary CreatePropertyCache(int capacity) public JsonPropertyInfo PolicyProperty { get; private set; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static bool TryIsPropertyRefEqual(in PropertyRef propertyRef, ReadOnlySpan propertyName, ulong key, ref JsonPropertyInfo info) + { + if (key == propertyRef.Key) + { + // We compare the whole name, although we could skip the first 7 bytes (but it's not any faster) + if (propertyName.Length <= PropertyNameKeyLength || + propertyName.SequenceEqual(propertyRef.Info.Name)) + { + info = propertyRef.Info; + return true; + } + } + + return false; + } + /// /// Get a key from the property name. /// The key consists of the first 7 bytes of the property name and then the length. @@ -424,13 +420,10 @@ public static ulong GetKey(ReadOnlySpan propertyName) { key = MemoryMarshal.Read(propertyName); - // Clear the high byte so it can hold the length. - key &= 0x00FFFFFFFFFFFFFF; - - // Use a length of 8. Any length > 7 (PropertyNameKeyLength) would work since - // the comparison logic tests for equality against the full contents instead of just - // the key if the property name is 8 or more characters. - key |= (ulong) 8 << (7 * BitsInByte); + // Max out the length byte. + // The comparison logic tests for equality against the full contents instead of just + // the key if the property name length is >7. + key |= 0xFF00000000000000; } else if (length > 3) { @@ -438,15 +431,15 @@ public static ulong GetKey(ReadOnlySpan propertyName) if (length == 7) { - key |= (ulong) propertyName[4] << (4 * BitsInByte) + key |= (ulong) propertyName[6] << (6 * BitsInByte) | (ulong) propertyName[5] << (5 * BitsInByte) - | (ulong) propertyName[6] << (6 * BitsInByte) + | (ulong) propertyName[4] << (4 * BitsInByte) | (ulong) 7 << (7 * BitsInByte); } else if (length == 6) { - key |= (ulong) propertyName[4] << (4 * BitsInByte) - | (ulong) propertyName[5] << (5 * BitsInByte) + key |= (ulong) propertyName[5] << (5 * BitsInByte) + | (ulong) propertyName[4] << (4 * BitsInByte) | (ulong) 6 << (7 * BitsInByte); } else if (length == 5) @@ -486,13 +479,13 @@ public static ulong GetKey(ReadOnlySpan propertyName) // Verify key contains the embedded bytes as expected. Debug.Assert( - length < 1 || propertyName[0] == (key & ((ulong)0xFF << 8 * 0)) >> 8 * 0 && - length < 2 || propertyName[1] == (key & ((ulong)0xFF << 8 * 1)) >> 8 * 1 && - length < 3 || propertyName[2] == (key & ((ulong)0xFF << 8 * 2)) >> 8 * 2 && - length < 4 || propertyName[3] == (key & ((ulong)0xFF << 8 * 3)) >> 8 * 3 && - length < 5 || propertyName[4] == (key & ((ulong)0xFF << 8 * 4)) >> 8 * 4 && - length < 6 || propertyName[5] == (key & ((ulong)0xFF << 8 * 5)) >> 8 * 5 && - length < 7 || propertyName[6] == (key & ((ulong)0xFF << 8 * 6)) >> 8 * 6); + (length < 1 || propertyName[0] == ((key & ((ulong)0xFF << 8 * 0)) >> 8 * 0)) && + (length < 2 || propertyName[1] == ((key & ((ulong)0xFF << 8 * 1)) >> 8 * 1)) && + (length < 3 || propertyName[2] == ((key & ((ulong)0xFF << 8 * 2)) >> 8 * 2)) && + (length < 4 || propertyName[3] == ((key & ((ulong)0xFF << 8 * 3)) >> 8 * 3)) && + (length < 5 || propertyName[4] == ((key & ((ulong)0xFF << 8 * 4)) >> 8 * 4)) && + (length < 6 || propertyName[5] == ((key & ((ulong)0xFF << 8 * 5)) >> 8 * 5)) && + (length < 7 || propertyName[6] == ((key & ((ulong)0xFF << 8 * 6)) >> 8 * 6))); return key; } diff --git a/src/System.Text.Json/tests/Serialization/PropertyNameTests.cs b/src/System.Text.Json/tests/Serialization/PropertyNameTests.cs index 3781fd6e6feb..c5feea340c20 100644 --- a/src/System.Text.Json/tests/Serialization/PropertyNameTests.cs +++ b/src/System.Text.Json/tests/Serialization/PropertyNameTests.cs @@ -311,7 +311,7 @@ private class ClassWithPropertyNamePermutations public int aaaaaaaa { get; set; } public int aaaaaaab { get; set; } - // 9 characters - caching code doesn't differentiate past this. + // 9 characters. public int aaaaaaaaa { get; set; } public int aaaaaaaab { get; set; } @@ -400,23 +400,34 @@ void Verify() } [Theory] - [InlineData(0x1)] - [InlineData(0x10)] - [InlineData(0x100)] - [InlineData(0x1000)] - [InlineData(0x10000)] - public static void LongPropertyNames(int propertyLength) + [InlineData(0x1, 'v')] + [InlineData(0x1, '\u0467')] + [InlineData(0x10, 'v')] + [InlineData(0x10, '\u0467')] + [InlineData(0x100, 'v')] + [InlineData(0x100, '\u0467')] + [InlineData(0x1000, 'v')] + [InlineData(0x1000, '\u0467')] + [InlineData(0x10000, 'v')] + [InlineData(0x10000, '\u0467')] + public static void LongPropertyNames(int propertyLength, char ch) { - // Although the CLR may limit member length to 1023, the serializer doesn't have a hard limit. + // Although the CLR may limit member length to 1023 bytes, the serializer doesn't have a hard limit. - string val = new string('v', propertyLength); + string val = new string(ch, propertyLength); string json = @"{""" + val + @""":1}"; EmptyClassWithExtensionProperty obj = JsonSerializer.Deserialize(json); Assert.True(obj.MyOverflow.ContainsKey(val)); - string jsonRoundTripped = JsonSerializer.Serialize(obj); + var options = new JsonSerializerOptions + { + // Avoid escaping '\u0467'. + Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping + }; + + string jsonRoundTripped = JsonSerializer.Serialize(obj, options); Assert.Equal(json, jsonRoundTripped); } } @@ -485,4 +496,3 @@ public class EmptyClassWithExtensionProperty public IDictionary MyOverflow { get; set; } } } -