Avoid serializer multi-threaded cache issues (dotnet/corefx#36549)

Commit migrated from https://github.com/dotnet/corefx/commit/3a4c464e80c17c3cd4b2e08358db7b457e187818

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.AddProperty.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.AddProperty.cs
index eece51e8e61e5dd0ee94dd3f4cf63cf1537dcfc5..ae416ac3d1dd72b9efe2e05b380aea543305e4e1 100644 (file)
--- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.AddProperty.cs
+++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.AddProperty.cs
@@ -43,12 +43,12 @@ namespace System.Text.Json.Serialization
                     ArrayPool<byte>.Shared.Return(tempArray);
                 }
 
-                _property_refs.Add(new PropertyRef(GetKey(propertyNameBytes), jsonInfo));
+                _propertyRefs.Add(new PropertyRef(GetKey(propertyNameBytes), jsonInfo));
             }
             else
             {
                 // A single property or an IEnumerable
-                _property_refs.Add(new PropertyRef(0, jsonInfo));
+                _propertyRefs.Add(new PropertyRef(0, jsonInfo));
             }
         }
 

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 a019705a5b1238d128d279d6b224bd6ddee94c60..572cace672f566ef18841fd34cc02aeb2d491cce 100644 (file)
--- 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
@@ -15,8 +15,10 @@ namespace System.Text.Json.Serialization
         // The length of the property name embedded in the key (in bytes).
         private const int PropertyNameKeyLength = 6;
 
-        private readonly List<PropertyRef> _property_refs = new List<PropertyRef>();
-        private readonly List<PropertyRef> _property_refs_sorted = new List<PropertyRef>();
+        private readonly List<PropertyRef> _propertyRefs = new List<PropertyRef>();
+
+        // Cache of properties by first ordering. Use an array for highest performance.
+        private volatile PropertyRef[] _propertyRefsSorted = null;
 
         internal delegate object ConstructorDelegate();
         internal ConstructorDelegate CreateObject { get; private set; }
@@ -28,6 +30,49 @@ namespace System.Text.Json.Serialization
 
         public Type Type { get; private set; }
 
+        internal void UpdateSortedPropertyCache(ref ReadStackFrame frame)
+        {
+            // Set the sorted property cache. Overwrite any existing cache which can occur in multi-threaded cases.
+            // Todo: on classes with many properties (about 50) we need to switch to a hashtable.
+            if (frame.PropertyRefCache != null)
+            {
+                List<PropertyRef> cache = frame.PropertyRefCache;
+
+                // Add any missing properties. This creates a consistent cache count which is important for
+                // the loop in GetProperty() when there are multiple threads in a race conditions each generating
+                // a cache for a given POCO type but with different property counts in the json.
+                while (cache.Count < _propertyRefs.Count)
+                {
+                    for (int iProperty = 0; iProperty < _propertyRefs.Count; iProperty++)
+                    {
+                        PropertyRef propertyRef = _propertyRefs[iProperty];
+
+                        bool found = false;
+                        int iCacheProperty = 0;
+                        for (; iCacheProperty < cache.Count; iCacheProperty++)
+                        {
+                            if (IsPropertyRefEqual(ref propertyRef, cache[iCacheProperty]))
+                            {
+                                // The property is already cached, skip to the next property.
+                                found = true;
+                                break;
+                            }
+                        }
+
+                        if (found == false)
+                        {
+                            cache.Add(propertyRef);
+                            break;
+                        }
+                    }
+                }
+
+                Debug.Assert(cache.Count == _propertyRefs.Count);
+                _propertyRefsSorted = cache.ToArray();
+                frame.PropertyRefCache = null;
+            }
+        }
+
         internal JsonClassInfo(Type type, JsonSerializerOptions options)
         {
             Type = type;
@@ -66,51 +111,73 @@ namespace System.Text.Json.Serialization
             }
         }
 
-        internal JsonPropertyInfo GetProperty(ReadOnlySpan<byte> propertyName, int propertyIndex)
+        internal JsonPropertyInfo GetProperty(ReadOnlySpan<byte> propertyName, ref ReadStackFrame frame)
         {
             ulong key = GetKey(propertyName);
             JsonPropertyInfo info = null;
 
             // First try sorted lookup.
-            int count = _property_refs_sorted.Count;
-            if (count != 0)
+            int propertyIndex = frame.PropertyIndex;
+
+            // If we're not trying to build the cache locally, and there is an existing cache, then use it.
+            bool hasPropertyCache = frame.PropertyRefCache == null && _propertyRefsSorted != null;
+            if (hasPropertyCache)
             {
-                int iForward = propertyIndex;
-                int iBackward = propertyIndex - 1;
-                while (iForward < count || (iBackward >= 0 && iBackward < count))
+                // This .Length is consistent no matter what json data intialized _propertyRefsSorted.
+                int count = _propertyRefsSorted.Length;
+                if (count != 0)
                 {
-                    if (iForward < count)
+                    int iForward = propertyIndex;
+                    int iBackward = propertyIndex - 1;
+                    while (iForward < count || iBackward >= 0)
                     {
-                        if (TryIsPropertyRefEqual(_property_refs_sorted, propertyName, key, iForward, out info))
+                        if (iForward < count)
                         {
-                            return info;
+                            if (TryIsPropertyRefEqual(ref _propertyRefsSorted[iForward], propertyName, key, ref info))
+                            {
+                                return info;
+                            }
+                            ++iForward;
                         }
-                        ++iForward;
-                    }
 
-                    if (iBackward >= 0)
-                    {
-                        if (TryIsPropertyRefEqual(_property_refs_sorted, propertyName, key, iBackward, out info))
+                        if (iBackward >= 0)
                         {
-                            return info;
+                            if (TryIsPropertyRefEqual(ref _propertyRefsSorted[iBackward], propertyName, key, ref info))
+                            {
+                                return info;
+                            }
+                            --iBackward;
                         }
-                        --iBackward;
                     }
                 }
             }
 
-            // Then try fallback
-            for (int i = 0; i < _property_refs.Count; i++)
+            // Try the main list which has all of the properties in a consistent order.
+            // We could get here even when hasPropertyCache==true if there is a race condition with different json
+            // property ordering and _propertyRefsSorted is re-assigned while in the loop above.
+            for (int i = 0; i < _propertyRefs.Count; i++)
             {
-                if (TryIsPropertyRefEqual(_property_refs, propertyName, key, i, out info))
+                PropertyRef propertyRef = _propertyRefs[i];
+                if (TryIsPropertyRefEqual(ref propertyRef, propertyName, key, ref info))
                 {
                     break;
                 }
             }
 
-            if (info != null)
+            if (!hasPropertyCache)
             {
-                _property_refs_sorted.Add(new PropertyRef(key, info));
+                if (propertyIndex == 0)
+                {
+                    // Create the temporary list on first property access to prevent a partially filled List.
+                    Debug.Assert(frame.PropertyRefCache == null);
+                    frame.PropertyRefCache = new List<PropertyRef>();
+                }
+
+                if (info != null)
+                {
+                    Debug.Assert(frame.PropertyRefCache != null);
+                    frame.PropertyRefCache.Add(new PropertyRef(key, info));
+                }
             }
 
             return info;
@@ -118,38 +185,51 @@ namespace System.Text.Json.Serialization
 
         internal JsonPropertyInfo GetPolicyProperty()
         {
-            Debug.Assert(_property_refs.Count == 1);
-            return _property_refs[0].Info;
+            Debug.Assert(_propertyRefs.Count == 1);
+            return _propertyRefs[0].Info;
         }
 
         internal JsonPropertyInfo GetProperty(int index)
         {
-            Debug.Assert(index < _property_refs.Count);
-            return _property_refs[index].Info;
+            Debug.Assert(index < _propertyRefs.Count);
+            return _propertyRefs[index].Info;
         }
 
         internal int PropertyCount
         {
             get
             {
-                return _property_refs.Count;
+                return _propertyRefs.Count;
             }
         }
 
-        private static bool TryIsPropertyRefEqual(List<PropertyRef> list, ReadOnlySpan<byte> propertyName, ulong key, int index, out JsonPropertyInfo info)
+        private static bool TryIsPropertyRefEqual(ref PropertyRef propertyRef, ReadOnlySpan<byte> propertyName, ulong key, ref JsonPropertyInfo info)
         {
-            if (key == list[index].Key)
+            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((ReadOnlySpan<byte>)list[index].Info._name))
+                    propertyName.SequenceEqual((ReadOnlySpan<byte>)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.SequenceEqual(other.Info.Name))
                 {
-                    info = list[index].Info;
                     return true;
                 }
             }
 
-            info = null;
             return false;
         }
 

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleObject.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleObject.cs
index b8470eb96b36ff752fe514f79fd12acdb5b53961..afaf9c4a4b576a8eb7ffb40fd899b674ad717667 100644 (file)
--- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleObject.cs
+++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleObject.cs
@@ -43,6 +43,8 @@ namespace System.Text.Json.Serialization
                 return isLastFrame;
             }
 
+            state.Current.JsonClassInfo.UpdateSortedPropertyCache(ref state.Current);
+
             object value = state.Current.ReturnValue;
 
             if (isLastFrame)

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs
index 6eeef9e0d6cd14be2874f0597f3a95fc868d6b9f..2f331de58d5f084bd0d2e67ceff9bd02bb5b476f 100644 (file)
--- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs
+++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.cs
@@ -64,7 +64,7 @@ namespace System.Text.Json.Serialization
                         Debug.Assert(state.Current.JsonClassInfo != default);
 
                         ReadOnlySpan<byte> propertyName = reader.HasValueSequence ? reader.ValueSequence.ToArray() : reader.ValueSpan;
-                        state.Current.JsonPropertyInfo = state.Current.JsonClassInfo.GetProperty(propertyName, state.Current.PropertyIndex);
+                        state.Current.JsonPropertyInfo = state.Current.JsonClassInfo.GetProperty(propertyName, ref state.Current);
                         if (state.Current.JsonPropertyInfo == null)
                         {
                             state.Current.JsonPropertyInfo = s_missingProperty;

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/PropertyRef.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/PropertyRef.cs
index 98d9d8f25f11fbbe5b28528b789d2a32422f8b56..187b38203dd34000117b2d011abfb9a343d631a8 100644 (file)
--- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/PropertyRef.cs
+++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/PropertyRef.cs
@@ -12,7 +12,9 @@ namespace System.Text.Json.Serialization
             Info = info;
         }
 
+        // The first 6 bytes are the first part of the name and last 2 bytes are the name's length.
         public readonly ulong Key;
+
         public readonly JsonPropertyInfo Info;
     }
 }

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs
index 376fdcbd12469df02f765ad9c9a232b82b593b72..1e6baf668792aa97df336c8e2de607bd38e859ef 100644 (file)
--- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs
+++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs
@@ -22,14 +22,18 @@ namespace System.Text.Json.Serialization
         // Support System.Array and other types that don't implement IList
         internal List<object> TempEnumerableValues;
 
-        // For performance, we order the properties by the first usage and this index helps find the right slot quicker.
+        // For performance, we order the properties by the first deserialize and PropertyIndex helps find the right slot quicker.
         internal int PropertyIndex;
+        internal List<PropertyRef> PropertyRefCache;
+
+        // The current JSON data for a property does not match a given POCO, so ignore the property (recursively for enumerables or object).
         internal bool Drain;
 
         internal void Reset()
         {
             ReturnValue = null;
             JsonClassInfo = null;
+            PropertyRefCache = null;
             PropertyIndex = 0;
             Drain = false;
             ResetProperty();

diff --git a/src/libraries/System.Text.Json/tests/Serialization/CacheTests.cs b/src/libraries/System.Text.Json/tests/Serialization/CacheTests.cs
new file mode 100644 (file)
index 0000000..31b0f45
--- /dev/null
+++ b/src/libraries/System.Text.Json/tests/Serialization/CacheTests.cs
@@ -0,0 +1,90 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// 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.Threading.Tasks;
+using Xunit;
+
+namespace System.Text.Json.Serialization.Tests
+{
+    public static class CacheTests
+    {
+        [Fact]
+        public static void MultipleThreads()
+        {
+            // Ensure no exceptions are thrown due to caching or other issues.
+            void SerializeAndDeserializeObject(bool useEmptyJson)
+            {
+                // Use localized caching
+                // Todo: localized caching not implemented yet. When implemented, add a run-time attribute to JsonSerializerOptions as that will create a separate cache held by JsonSerializerOptions.
+                var options = new JsonSerializerOptions();
+
+                string json;
+
+                if (useEmptyJson)
+                {
+                    json = "{}";
+                }
+                else
+                {
+                    SimpleTestClass testObj = new SimpleTestClass();
+                    testObj.Initialize();
+                    testObj.Verify();
+
+                    json = JsonSerializer.ToString(testObj, options);
+                }
+
+                SimpleTestClass testObjDeserialized = JsonSerializer.Parse<SimpleTestClass>(json, options);
+                testObjDeserialized.Verify();
+            };
+
+            Task[] tasks = new Task[8];
+            bool useEmptyJson = false;
+            for (int i = 0; i < tasks.Length; i++)
+            {
+                tasks[i] = Task.Run(() => SerializeAndDeserializeObject(useEmptyJson));
+                useEmptyJson = !useEmptyJson;
+            };
+
+            Task.WaitAll(tasks);
+        }
+
+        [Fact]
+        public static void PropertyCacheWithMinInputsFirst()
+        {
+            // Use localized caching
+            // Todo: localized caching not implemented yet. When implemented, add a run-time attribute to JsonSerializerOptions as that will create a separate cache held by JsonSerializerOptions.
+            var options = new JsonSerializerOptions();
+
+            string json = "{}";
+            JsonSerializer.Parse<SimpleTestClass>(json, options);
+
+            SimpleTestClass testObj = new SimpleTestClass();
+            testObj.Initialize();
+            testObj.Verify();
+
+            json = JsonSerializer.ToString(testObj, options);
+            testObj = JsonSerializer.Parse<SimpleTestClass>(json, options);
+            testObj.Verify();
+        }
+
+        [Fact]
+        public static void PropertyCacheWithMinInputsLast()
+        {
+            // Use localized caching
+            // Todo: localized caching not implemented yet. When implemented, add a run-time attribute to JsonSerializerOptions as that will create a separate cache held by JsonSerializerOptions.
+            var options = new JsonSerializerOptions();
+
+            SimpleTestClass testObj = new SimpleTestClass();
+            testObj.Initialize();
+            testObj.Verify();
+
+            string json = JsonSerializer.ToString(testObj, options);
+            testObj = JsonSerializer.Parse<SimpleTestClass>(json, options);
+            testObj.Verify();
+
+            json = "{}";
+            JsonSerializer.Parse<SimpleTestClass>(json, options);
+        }
+    }
+}

diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj
index 7ed09a316b31a7c20cd23834f88543aec3c2829f..649d7671712c8ae157030d8417fc782f2cf90622 100644 (file)
--- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj
+++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj
@@ -24,6 +24,7 @@
     <Compile Include="ResizableArray.cs" />
     <Compile Include="Serialization\Array.ReadTests.cs" />
     <Compile Include="Serialization\Array.WriteTests.cs" />
+    <Compile Include="Serialization\CacheTests.cs" />
     <Compile Include="Serialization\CyclicTests.cs" />
     <Compile Include="Serialization\EnumTests.cs" />
     <Compile Include="Serialization\Null.ReadTests.cs" />