From 8cb79130e4dde9c2157410e5a3caffdfd089c6b6 Mon Sep 17 00:00:00 2001 From: Eduardo Velarde <32459232+eduardo-vp@users.noreply.github.com> Date: Tue, 27 Aug 2024 18:53:10 -0700 Subject: [PATCH 1/3] Stop counting work items from ThreadPoolTypedWorkItemQueue for ThreadPool.CompletedWorkItemCount (#106854) * Stop counting work items from ThreadPoolTypedWorkItemQueue as completed work items * Fix CompletedWorkItemCount * Update src/libraries/System.Threading.ThreadPool/tests/ThreadPoolTests.cs Co-authored-by: Koundinya Veluri * Run CompletedWorkItemCountTest on Windows only --------- Co-authored-by: Eduardo Manuel Velarde Polar Co-authored-by: Koundinya Veluri --- .../System/Threading/ThreadPoolWorkQueue.cs | 6 ++++- .../tests/ThreadPoolTests.cs | 27 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs index 6fa669046a1f06..f8bf0a43fe8c2e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs @@ -1414,7 +1414,11 @@ void IThreadPoolWorkItem.Execute() currentThread.ResetThreadPoolThread(); } - ThreadInt64PersistentCounter.Add(tl.threadLocalCompletionCountObject!, completedCount); + // Discount a work item here to avoid counting most of the queue processing work items + if (completedCount > 1) + { + ThreadInt64PersistentCounter.Add(tl.threadLocalCompletionCountObject!, completedCount - 1); + } } } diff --git a/src/libraries/System.Threading.ThreadPool/tests/ThreadPoolTests.cs b/src/libraries/System.Threading.ThreadPool/tests/ThreadPoolTests.cs index f9e454abbe8a64..1e078971088f47 100644 --- a/src/libraries/System.Threading.ThreadPool/tests/ThreadPoolTests.cs +++ b/src/libraries/System.Threading.ThreadPool/tests/ThreadPoolTests.cs @@ -1462,6 +1462,33 @@ static async Task RunAsyncIOTest() }, ioCompletionPortCount.ToString()).Dispose(); } + + [ConditionalFact(nameof(IsThreadingAndRemoteExecutorSupported))] + [PlatformSpecific(TestPlatforms.Windows)] + public static unsafe void ThreadPoolCompletedWorkItemCountTest() + { + // Run in a separate process to test in a clean thread pool environment such that we don't count external work items + RemoteExecutor.Invoke(() => + { + using var manualResetEvent = new ManualResetEventSlim(false); + + var overlapped = new Overlapped(); + NativeOverlapped* nativeOverlapped = overlapped.Pack((errorCode, numBytes, innerNativeOverlapped) => + { + Overlapped.Free(innerNativeOverlapped); + manualResetEvent.Set(); + }, null); + + ThreadPool.UnsafeQueueNativeOverlapped(nativeOverlapped); + manualResetEvent.Wait(); + + // Allow work item(s) to be marked as completed during this time, should be only one + ThreadTestHelpers.WaitForCondition(() => ThreadPool.CompletedWorkItemCount == 1); + Thread.Sleep(50); + Assert.Equal(1, ThreadPool.CompletedWorkItemCount); + }).Dispose(); + } + public static bool IsThreadingAndRemoteExecutorSupported => PlatformDetection.IsThreadingSupported && RemoteExecutor.IsSupported; From 427d1550d2daa8047df63b0589a3e71940a8906f Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Tue, 18 Feb 2025 10:07:17 -0800 Subject: [PATCH 2/3] Make counting of IO completion work items more precise on Windows - Follow-up to https://github.com/dotnet/runtime/pull/106854. Issue: https://github.com/dotnet/runtime/issues/104284. - Before the change, the modified test case often yields 5 or 6 completed work items, due to queue-processing work items that happen to not process any user work items. After the change, it always yields 4. - Looks like it doesn't hurt to have more-precise counting, and there was a request to backport a fix to .NET 8, where it's more necessary to fix the issue --- .../Threading/ThreadInt64PersistentCounter.cs | 23 ++++++++++++-- .../src/System/Threading/ThreadPool.Unix.cs | 15 ++++++++- .../System/Threading/ThreadPool.Windows.cs | 18 ++++++++++- .../System/Threading/ThreadPoolWorkQueue.cs | 7 ++++- .../tests/ThreadPoolTests.cs | 31 ++++++++++++------- 5 files changed, 78 insertions(+), 16 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadInt64PersistentCounter.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadInt64PersistentCounter.cs index 0f7fbc06a9a8eb..bba3ee1dd60752 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadInt64PersistentCounter.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadInt64PersistentCounter.cs @@ -31,6 +31,13 @@ public static void Increment(object threadLocalCountObject) Unsafe.As(threadLocalCountObject).Increment(); } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static void Decrement(object threadLocalCountObject) + { + Debug.Assert(threadLocalCountObject is ThreadLocalNode); + Unsafe.As(threadLocalCountObject).Decrement(); + } + [MethodImpl(MethodImplOptions.AggressiveInlining)] public static void Add(object threadLocalCountObject, uint count) { @@ -134,6 +141,18 @@ public void Increment() OnAddOverflow(1); } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public void Decrement() + { + if (_count != 0) + { + _count--; + return; + } + + OnAddOverflow(-1); + } + public void Add(uint count) { Debug.Assert(count != 0); @@ -149,7 +168,7 @@ public void Add(uint count) } [MethodImpl(MethodImplOptions.NoInlining)] - private void OnAddOverflow(uint count) + private void OnAddOverflow(long count) { Debug.Assert(count != 0); @@ -161,7 +180,7 @@ private void OnAddOverflow(uint count) counter._lock.Acquire(); try { - counter._overflowCount += (long)_count + count; + counter._overflowCount += _count + count; _count = 0; } finally diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Unix.cs index 83faa720e3974a..69d604f3f82264 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Unix.cs @@ -131,7 +131,20 @@ public static long CompletedWorkItemCount { get { - return PortableThreadPool.ThreadPoolInstance.CompletedWorkItemCount; + long count = PortableThreadPool.ThreadPoolInstance.CompletedWorkItemCount; + + // Ensure that the returned value is monotonically increasing + long lastCount = s_lastCompletedWorkItemCount; + if (count > lastCount) + { + s_lastCompletedWorkItemCount = count; + } + else + { + count = lastCount; + } + + return count; } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Windows.cs index 0da875498afc18..b0108b0ea5dc8a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Windows.cs @@ -201,7 +201,23 @@ public static long CompletedWorkItemCount { get { - return ThreadPool.UseWindowsThreadPool ? WindowsThreadPool.CompletedWorkItemCount : PortableThreadPool.ThreadPoolInstance.CompletedWorkItemCount; + long count = + UseWindowsThreadPool + ? WindowsThreadPool.CompletedWorkItemCount + : PortableThreadPool.ThreadPoolInstance.CompletedWorkItemCount; + + // Ensure that the returned value is monotonically increasing + long lastCount = s_lastCompletedWorkItemCount; + if (count > lastCount) + { + s_lastCompletedWorkItemCount = count; + } + else + { + count = lastCount; + } + + return count; } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs index f8bf0a43fe8c2e..125cefc734907b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs @@ -1375,6 +1375,9 @@ void IThreadPoolWorkItem.Execute() Debug.Assert(stageBeforeUpdate != QueueProcessingStage.NotScheduled); if (stageBeforeUpdate == QueueProcessingStage.Determining) { + // Discount a work item here to avoid counting this queue processing work item + ThreadInt64PersistentCounter.Decrement( + ThreadPoolWorkQueueThreadLocals.threadLocals!.threadLocalCompletionCountObject!); return; } } @@ -1414,7 +1417,7 @@ void IThreadPoolWorkItem.Execute() currentThread.ResetThreadPoolThread(); } - // Discount a work item here to avoid counting most of the queue processing work items + // Discount a work item here to avoid counting this queue processing work item if (completedCount > 1) { ThreadInt64PersistentCounter.Add(tl.threadLocalCompletionCountObject!, completedCount - 1); @@ -1618,6 +1621,8 @@ public static partial class ThreadPool internal static readonly ThreadPoolWorkQueue s_workQueue = new ThreadPoolWorkQueue(); + private static long s_lastCompletedWorkItemCount; + /// Shim used to invoke of the supplied . internal static readonly Action s_invokeAsyncStateMachineBox = static state => { diff --git a/src/libraries/System.Threading.ThreadPool/tests/ThreadPoolTests.cs b/src/libraries/System.Threading.ThreadPool/tests/ThreadPoolTests.cs index 1e078971088f47..a59997f8e3f8b3 100644 --- a/src/libraries/System.Threading.ThreadPool/tests/ThreadPoolTests.cs +++ b/src/libraries/System.Threading.ThreadPool/tests/ThreadPoolTests.cs @@ -1470,22 +1470,31 @@ public static unsafe void ThreadPoolCompletedWorkItemCountTest() // Run in a separate process to test in a clean thread pool environment such that we don't count external work items RemoteExecutor.Invoke(() => { - using var manualResetEvent = new ManualResetEventSlim(false); + const int WorkItemCount = 4; - var overlapped = new Overlapped(); - NativeOverlapped* nativeOverlapped = overlapped.Pack((errorCode, numBytes, innerNativeOverlapped) => + int completedWorkItemCount = 0; + using var allWorkItemsCompleted = new AutoResetEvent(false); + + IOCompletionCallback callback = + (errorCode, numBytes, innerNativeOverlapped) => + { + Overlapped.Free(innerNativeOverlapped); + if (Interlocked.Increment(ref completedWorkItemCount) == WorkItemCount) + { + allWorkItemsCompleted.Set(); + } + }; + for (int i = 0; i < WorkItemCount; i++) { - Overlapped.Free(innerNativeOverlapped); - manualResetEvent.Set(); - }, null); + ThreadPool.UnsafeQueueNativeOverlapped(new Overlapped().Pack(callback, null)); + } - ThreadPool.UnsafeQueueNativeOverlapped(nativeOverlapped); - manualResetEvent.Wait(); + allWorkItemsCompleted.CheckedWait(); - // Allow work item(s) to be marked as completed during this time, should be only one - ThreadTestHelpers.WaitForCondition(() => ThreadPool.CompletedWorkItemCount == 1); + // Allow work items to be marked as completed during this time + ThreadTestHelpers.WaitForCondition(() => ThreadPool.CompletedWorkItemCount >= WorkItemCount); Thread.Sleep(50); - Assert.Equal(1, ThreadPool.CompletedWorkItemCount); + Assert.Equal(WorkItemCount, ThreadPool.CompletedWorkItemCount); }).Dispose(); } From 3316f112c6770801c57fbccd1373b0d359b0ea94 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Fri, 21 Feb 2025 10:31:25 -0800 Subject: [PATCH 3/3] Address feedback --- .../Threading/ThreadInt64PersistentCounter.cs | 12 ++++++++++++ .../src/System/Threading/ThreadPool.Unix.cs | 15 +-------------- .../src/System/Threading/ThreadPool.Windows.cs | 18 +----------------- .../System/Threading/ThreadPoolWorkQueue.cs | 2 -- 4 files changed, 14 insertions(+), 33 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadInt64PersistentCounter.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadInt64PersistentCounter.cs index bba3ee1dd60752..29cf2dce305657 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadInt64PersistentCounter.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadInt64PersistentCounter.cs @@ -15,6 +15,7 @@ internal sealed class ThreadInt64PersistentCounter private static List? t_nodeFinalizationHelpers; private long _overflowCount; + private long _lastReturnedCount; // dummy node serving as a start and end of the ring list private readonly ThreadLocalNode _nodes; @@ -83,6 +84,17 @@ public long Count count += node.Count; node = node._next; } + + // Ensure that the returned value is monotonically increasing + long lastReturnedCount = _lastReturnedCount; + if (count > lastReturnedCount) + { + _lastReturnedCount = count; + } + else + { + count = lastReturnedCount; + } } finally { diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Unix.cs index 69d604f3f82264..83faa720e3974a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Unix.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Unix.cs @@ -131,20 +131,7 @@ public static long CompletedWorkItemCount { get { - long count = PortableThreadPool.ThreadPoolInstance.CompletedWorkItemCount; - - // Ensure that the returned value is monotonically increasing - long lastCount = s_lastCompletedWorkItemCount; - if (count > lastCount) - { - s_lastCompletedWorkItemCount = count; - } - else - { - count = lastCount; - } - - return count; + return PortableThreadPool.ThreadPoolInstance.CompletedWorkItemCount; } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Windows.cs index b0108b0ea5dc8a..0da875498afc18 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Windows.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Windows.cs @@ -201,23 +201,7 @@ public static long CompletedWorkItemCount { get { - long count = - UseWindowsThreadPool - ? WindowsThreadPool.CompletedWorkItemCount - : PortableThreadPool.ThreadPoolInstance.CompletedWorkItemCount; - - // Ensure that the returned value is monotonically increasing - long lastCount = s_lastCompletedWorkItemCount; - if (count > lastCount) - { - s_lastCompletedWorkItemCount = count; - } - else - { - count = lastCount; - } - - return count; + return ThreadPool.UseWindowsThreadPool ? WindowsThreadPool.CompletedWorkItemCount : PortableThreadPool.ThreadPoolInstance.CompletedWorkItemCount; } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs index 125cefc734907b..7660d427da63fd 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs @@ -1621,8 +1621,6 @@ public static partial class ThreadPool internal static readonly ThreadPoolWorkQueue s_workQueue = new ThreadPoolWorkQueue(); - private static long s_lastCompletedWorkItemCount; - /// Shim used to invoke of the supplied . internal static readonly Action s_invokeAsyncStateMachineBox = static state => {