From 05d48bbde2a109ab2f880bda1ecabd761d257646 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Fri, 7 Dec 2018 11:43:13 -0500 Subject: [PATCH 1/3] Remove some unnecessary spinning Most of the use of SpinWait in CoreLib involves waiting for some short-lived operation to complete on another thread, in which case the spinning thread should backoff as it's unable to make forward progress until the other operation completes. In a few cases, however, SpinWait is being used just around CompareExchange operations, such that at least one thread running this code path is guaranteed to make forward progress, and the backoff in the spinning doesn't actually help (in theory it could help to reduce contention if lots of threads were all trying to CompareExchange concurrently, but in such cases you'd actually want more randomized backoff, as otherwise it's likely all the threads would re-attempt at around the same time and similarly re-encounter contention). --- .../System/Collections/Concurrent/ConcurrentQueueSegment.cs | 2 -- .../src/System/Threading/Tasks/Task.cs | 6 ------ 2 files changed, 8 deletions(-) diff --git a/src/System.Private.CoreLib/shared/System/Collections/Concurrent/ConcurrentQueueSegment.cs b/src/System.Private.CoreLib/shared/System/Collections/Concurrent/ConcurrentQueueSegment.cs index c706fae02e13..6c78b932b014 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/Concurrent/ConcurrentQueueSegment.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/Concurrent/ConcurrentQueueSegment.cs @@ -112,7 +112,6 @@ internal static int RoundUpToPowerOf2(int i) _frozenForEnqueues = true; // Increase the tail by FreezeOffset, spinning until we're successful in doing so. - var spinner = new SpinWait(); while (true) { int tail = Volatile.Read(ref _headAndTail.Tail); @@ -120,7 +119,6 @@ internal static int RoundUpToPowerOf2(int i) { break; } - spinner.SpinOnce(); } } } diff --git a/src/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs b/src/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs index 4b9ccc630000..857bcdbf6b4f 100644 --- a/src/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs +++ b/src/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs @@ -721,7 +721,6 @@ internal bool AtomicStateUpdate(int newBits, int illegalBits) private bool AtomicStateUpdateSlow(int newBits, int illegalBits) { - var sw = new SpinWait(); do { int oldFlags = m_stateFlags; @@ -730,13 +729,11 @@ private bool AtomicStateUpdateSlow(int newBits, int illegalBits) { return true; } - sw.SpinOnce(); } while (true); } internal bool AtomicStateUpdate(int newBits, int illegalBits, ref int oldFlags) { - SpinWait sw = new SpinWait(); do { oldFlags = m_stateFlags; @@ -745,7 +742,6 @@ internal bool AtomicStateUpdate(int newBits, int illegalBits, ref int oldFlags) { return true; } - sw.SpinOnce(); } while (true); } @@ -772,13 +768,11 @@ internal void SetNotificationForWaitCompletion(bool enabled) else { // Atomically clear the END_AWAIT_NOTIFICATION bit - SpinWait sw = new SpinWait(); while (true) { int oldFlags = m_stateFlags; int newFlags = oldFlags & (~TASK_STATE_WAIT_COMPLETION_NOTIFICATION); if (Interlocked.CompareExchange(ref m_stateFlags, newFlags, oldFlags) == oldFlags) break; - sw.SpinOnce(); } } } From 74af7ae8668064f5ea55595d900f464612e6ff46 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Fri, 7 Dec 2018 14:11:41 -0500 Subject: [PATCH 2/3] Address PR feedback --- .../Concurrent/ConcurrentQueueSegment.cs | 6 +++-- .../src/System/Threading/Tasks/Task.cs | 23 +++++++++++-------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/System.Private.CoreLib/shared/System/Collections/Concurrent/ConcurrentQueueSegment.cs b/src/System.Private.CoreLib/shared/System/Collections/Concurrent/ConcurrentQueueSegment.cs index 6c78b932b014..e7bc3b84e74f 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/Concurrent/ConcurrentQueueSegment.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/Concurrent/ConcurrentQueueSegment.cs @@ -112,13 +112,15 @@ internal static int RoundUpToPowerOf2(int i) _frozenForEnqueues = true; // Increase the tail by FreezeOffset, spinning until we're successful in doing so. + int tail = Volatile.Read(ref _headAndTail.Tail); while (true) { - int tail = Volatile.Read(ref _headAndTail.Tail); - if (Interlocked.CompareExchange(ref _headAndTail.Tail, tail + FreezeOffset, tail) == tail) + int oldTail = Interlocked.CompareExchange(ref _headAndTail.Tail, tail + FreezeOffset, tail); + if (oldTail == tail) { break; } + tail = oldTail; } } } diff --git a/src/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs b/src/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs index 857bcdbf6b4f..b9b60dd7a28d 100644 --- a/src/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs +++ b/src/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs @@ -721,27 +721,31 @@ internal bool AtomicStateUpdate(int newBits, int illegalBits) private bool AtomicStateUpdateSlow(int newBits, int illegalBits) { + int flags = m_stateFlags; do { - int oldFlags = m_stateFlags; - if ((oldFlags & illegalBits) != 0) return false; - if (Interlocked.CompareExchange(ref m_stateFlags, oldFlags | newBits, oldFlags) == oldFlags) + if ((flags & illegalBits) != 0) return false; + int oldFlags = Interlocked.CompareExchange(ref m_stateFlags, flags | newBits, flags); + if (oldFlags == flags) { return true; } + flags = oldFlags; } while (true); } internal bool AtomicStateUpdate(int newBits, int illegalBits, ref int oldFlags) { + int flags = oldFlags = m_stateFlags; do { - oldFlags = m_stateFlags; - if ((oldFlags & illegalBits) != 0) return false; - if (Interlocked.CompareExchange(ref m_stateFlags, oldFlags | newBits, oldFlags) == oldFlags) + if ((flags & illegalBits) != 0) return false; + oldFlags = Interlocked.CompareExchange(ref m_stateFlags, flags | newBits, flags); + if (oldFlags == flags) { return true; } + flags = oldFlags; } while (true); } @@ -768,11 +772,12 @@ internal void SetNotificationForWaitCompletion(bool enabled) else { // Atomically clear the END_AWAIT_NOTIFICATION bit + int flags = m_stateFlags; while (true) { - int oldFlags = m_stateFlags; - int newFlags = oldFlags & (~TASK_STATE_WAIT_COMPLETION_NOTIFICATION); - if (Interlocked.CompareExchange(ref m_stateFlags, newFlags, oldFlags) == oldFlags) break; + int oldFlags = Interlocked.CompareExchange(ref m_stateFlags, flags & (~TASK_STATE_WAIT_COMPLETION_NOTIFICATION), flags); + if (oldFlags == flags) break; + flags = oldFlags; } } } From 7d93d801873b0ad8c1b3a7c343590871903d9c15 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Fri, 7 Dec 2018 16:53:11 -0500 Subject: [PATCH 3/3] Address more PR feedback --- .../System/Collections/Concurrent/ConcurrentQueueSegment.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Private.CoreLib/shared/System/Collections/Concurrent/ConcurrentQueueSegment.cs b/src/System.Private.CoreLib/shared/System/Collections/Concurrent/ConcurrentQueueSegment.cs index e7bc3b84e74f..85224a537a5f 100644 --- a/src/System.Private.CoreLib/shared/System/Collections/Concurrent/ConcurrentQueueSegment.cs +++ b/src/System.Private.CoreLib/shared/System/Collections/Concurrent/ConcurrentQueueSegment.cs @@ -112,7 +112,7 @@ internal static int RoundUpToPowerOf2(int i) _frozenForEnqueues = true; // Increase the tail by FreezeOffset, spinning until we're successful in doing so. - int tail = Volatile.Read(ref _headAndTail.Tail); + int tail = _headAndTail.Tail; while (true) { int oldTail = Interlocked.CompareExchange(ref _headAndTail.Tail, tail + FreezeOffset, tail);