Improve ArrayPool core affinity (#8716)

- Move the ExecutionId to non-generic type so that it can be shared by all ArrayPool instances.
- Add logic to refresh it periodically. It avoids pathological cases where the OS scheduler ends up reassigns the preferred cores and multiple active threads start competing over the same buckets.
- Removed flushing of ExecutionId on LockedStack lock contention since it was not very effective

5 files changed, 47 insertions, 90 deletions

diff --git a/src/mscorlib/corefx/System/Buffers/TlsOverPerCoreLockedStacksArrayPool.Unix.cs b/src/mscorlib/corefx/System/Buffers/TlsOverPerCoreLockedStacksArrayPool.Unix.cs
deleted file mode 100644
index 8a1d006b12..0000000000
--- a/src/mscorlib/corefx/System/Buffers/TlsOverPerCoreLockedStacksArrayPool.Unix.cs
+++ /dev/null
@@ -1,28 +0,0 @@
-// 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 Microsoft.Win32;
-using System.Runtime.CompilerServices;
-
-namespace System.Buffers
-{
-    internal sealed partial class TlsOverPerCoreLockedStacksArrayPool<T>
-    {
-        /// <summary>Get an identifier for the current thread to use to index into the stacks.</summary>
-        private static int ExecutionId
-        {
-            [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get
-            {
-                // On Unix, CurrentProcessorNumber is implemented in terms of sched_getcpu, which
-                // doesn't exist on all platforms.  On those it doesn't exist on, GetCurrentProcessorNumber
-                // returns -1.  As a fallback in that case and to spread the threads across the buckets
-                // by default, we use the current managed thread ID as a proxy.
-                int id = CurrentProcessorNumber;
-                if (id < 0) id = Environment.CurrentManagedThreadId;
-                return id;
-            }
-        }
-    }
-}
diff --git a/src/mscorlib/corefx/System/Buffers/TlsOverPerCoreLockedStacksArrayPool.Windows.cs b/src/mscorlib/corefx/System/Buffers/TlsOverPerCoreLockedStacksArrayPool.Windows.cs
deleted file mode 100644
index d42242c910..0000000000
--- a/src/mscorlib/corefx/System/Buffers/TlsOverPerCoreLockedStacksArrayPool.Windows.cs
+++ /dev/null
@@ -1,20 +0,0 @@
-// 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 Microsoft.Win32;
-using System.Runtime.CompilerServices;
-using System.Threading;
-
-namespace System.Buffers
-{
-    internal sealed partial class TlsOverPerCoreLockedStacksArrayPool<T> : ArrayPool<T>
-    {
-        /// <summary>Get an identifier for the current thread to use to index into the stacks.</summary>
-        private static int ExecutionId
-        {
-            [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get { return CurrentProcessorNumber; }
-        }
-    }
-}
diff --git a/src/mscorlib/corefx/System/Buffers/TlsOverPerCoreLockedStacksArrayPool.cs b/src/mscorlib/corefx/System/Buffers/TlsOverPerCoreLockedStacksArrayPool.cs
index debc33615f..9cf8bad487 100644
--- a/src/mscorlib/corefx/System/Buffers/TlsOverPerCoreLockedStacksArrayPool.cs
+++ b/src/mscorlib/corefx/System/Buffers/TlsOverPerCoreLockedStacksArrayPool.cs
@@ -45,11 +45,6 @@ namespace System.Buffers
         /// <summary>A per-thread array of arrays, to cache one array per array size per thread.</summary>
         [ThreadStatic]
         private static T[][] t_tlsBuckets;
-        /// <summary>
-        /// Cached processor number used as a hint for which per-core stack to access.
-        /// </summary>
-        [ThreadStatic]
-        private static int? t_cachedProcessorNumber;
 
         /// <summary>Initialize the pool.</summary>
         public TlsOverPerCoreLockedStacksArrayPool()
@@ -72,22 +67,6 @@ namespace System.Buffers
         /// <summary>Gets an ID for the pool to use with events.</summary>
         private int Id => GetHashCode();
 
-        /// <summary>Gets the processor number associated with the current thread.</summary>
-        /// <remarks>Uses a cached value if one exists on the current thread.</remarks>
-        private static int CurrentProcessorNumber
-        {
-            [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get
-            {
-                int? num = t_cachedProcessorNumber;
-                if (!num.HasValue)
-                {
-                    t_cachedProcessorNumber = num = Environment.CurrentProcessorNumber;
-                }
-                return num.GetValueOrDefault();
-            }
-        }
-
         public override T[] Rent(int minimumLength)
         {
             // Arrays can't be smaller than zero.  We allow requesting zero-length arrays (even though
@@ -249,7 +228,7 @@ namespace System.Buffers
                 // Try to push on to the associated stack first.  If that fails,
                 // round-robin through the other stacks.
                 LockedStack[] stacks = _perCoreStacks;
-                int index = ExecutionId % stacks.Length;
+                int index = Environment.CurrentExecutionId % stacks.Length;
                 for (int i = 0; i < stacks.Length; i++)
                 {
                     if (stacks[index].TryPush(array)) return;
@@ -265,7 +244,7 @@ namespace System.Buffers
                 // round-robin through the other stacks.
                 T[] arr;
                 LockedStack[] stacks = _perCoreStacks;
-                int index = ExecutionId % stacks.Length;
+                int index = Environment.CurrentExecutionId % stacks.Length;
                 for (int i = 0; i < stacks.Length; i++)
                 {
                     if ((arr = stacks[index].TryPop()) != null) return arr;
@@ -285,7 +264,7 @@ namespace System.Buffers
             public bool TryPush(T[] array)
             {
                 bool enqueued = false;
-                MonitorEnterWithProcNumberFlush(this);
+                Monitor.Enter(this);
                 if (_count < MaxBuffersPerArraySizePerCore)
                 {
                     _arrays[_count++] = array;
@@ -299,7 +278,7 @@ namespace System.Buffers
             public T[] TryPop()
             {
                 T[] arr = null;
-                MonitorEnterWithProcNumberFlush(this);
+                Monitor.Enter(this);
                 if (_count > 0)
                 {
                     arr = _arrays[--_count];
@@ -308,21 +287,6 @@ namespace System.Buffers
                 Monitor.Exit(this);
                 return arr;
             }
-
-            /// <summary>
-            /// Enters the monitor on the object.  If there is any contention while trying
-            /// to acquire the monitor, it flushes the cached processor number so that subsequent
-            /// attempts to access the per-core stacks will use an updated processor number.
-            /// </summary>
-            [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            private static void MonitorEnterWithProcNumberFlush(object obj)
-            {
-                if (!Monitor.TryEnter(obj))
-                {
-                    t_cachedProcessorNumber = null;
-                    Monitor.Enter(obj);
-                }
-            }
         }
     }
 }
diff --git a/src/mscorlib/mscorlib.shared.sources.props b/src/mscorlib/mscorlib.shared.sources.props
index 5bab030280..c78229d54d 100644
--- a/src/mscorlib/mscorlib.shared.sources.props
+++ b/src/mscorlib/mscorlib.shared.sources.props
@@ -1172,8 +1172,6 @@
     <BuffersSources Include="$(CoreFxSourcesRoot)\System\Buffers\ArrayPoolEventSource.cs" />
     <BuffersSources Include="$(CoreFxSourcesRoot)\System\Buffers\ConfigurableArrayPool.cs" />
     <BuffersSources Include="$(CoreFxSourcesRoot)\System\Buffers\TlsOverPerCoreLockedStacksArrayPool.cs" />
-    <BuffersSources Include="$(CoreFxSourcesRoot)\System\Buffers\TlsOverPerCoreLockedStacksArrayPool.Windows.cs" Condition="'$(TargetsUnix)' != 'true'" />
-    <BuffersSources Include="$(CoreFxSourcesRoot)\System\Buffers\TlsOverPerCoreLockedStacksArrayPool.Unix.cs" Condition="'$(TargetsUnix)' == 'true'" />
     <BuffersSources Include="$(CoreFxSourcesRoot)\System\Buffers\Utilities.cs" />
     <SecuritySources Include="$(CoreFxSourcesRoot)\System\Security\CryptographicException.cs" />
   </ItemGroup>
diff --git a/src/mscorlib/src/System/Environment.cs b/src/mscorlib/src/System/Environment.cs
index 835219a01c..81c1f09820 100644
--- a/src/mscorlib/src/System/Environment.cs
+++ b/src/mscorlib/src/System/Environment.cs
@@ -1144,6 +1144,49 @@ namespace System {
             get;
         }
 
+        // The upper bits of t_executionIdCache are the executionId. The lower bits of
+        // the t_executionIdCache are counting down to get it periodically refreshed.
+        // TODO: Consider flushing the executionIdCache on Wait operations or similar 
+        // actions that are likely to result in changing the executing core
+        [ThreadStatic]
+        static int t_executionIdCache;
+
+        const int ExecutionIdCacheShift = 16;
+        const int ExecutionIdCacheCountDownMask = (1 << ExecutionIdCacheShift) - 1;
+        const int ExecutionIdRefreshRate = 5000;
+
+        private static int RefreshExecutionId()
+        {
+            int executionId = CurrentProcessorNumber;
+
+            // On Unix, CurrentProcessorNumber is implemented in terms of sched_getcpu, which
+            // doesn't exist on all platforms.  On those it doesn't exist on, GetCurrentProcessorNumber
+            // returns -1.  As a fallback in that case and to spread the threads across the buckets
+            // by default, we use the current managed thread ID as a proxy.
+            if (executionId < 0) executionId = Environment.CurrentManagedThreadId;
+
+            Debug.Assert(ExecutionIdRefreshRate <= ExecutionIdCacheCountDownMask);
+
+            // Mask with Int32.MaxValue to ensure the execution Id is not negative
+            t_executionIdCache = ((executionId << ExecutionIdCacheShift) & Int32.MaxValue) + ExecutionIdRefreshRate;
+
+            return executionId;
+        }
+
+        // Cached processor number used as a hint for which per-core stack to access. It is periodically
+        // refreshed to trail the actual thread core affinity.
+        internal static int CurrentExecutionId
+        {
+            [MethodImpl(MethodImplOptions.AggressiveInlining)]
+            get
+            {
+                int executionIdCache = t_executionIdCache--;
+                if ((executionIdCache & ExecutionIdCacheCountDownMask) == 0)
+                    return RefreshExecutionId();
+                return (executionIdCache >> ExecutionIdCacheShift);
+            }
+        }
+
         public static string GetEnvironmentVariable(string variable)
         {
             if (variable == null)