From: Stephen Toub Date: Wed, 1 Feb 2017 16:51:27 +0000 (-0500) Subject: Miscellaneous code cleanup X-Git-Tag: submit/tizen/20210909.063632~11030^2~8246^2~1 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=094cd9bb6cfcc316190a1662b08e562c6a745177;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Miscellaneous code cleanup - Remove some explicit ctors - Remove some unnecessary casts - Mark some fields readonly - Follow style guidelines for visibility/static ordering in signatures - Move usings to top of file - Delete some stale comments - Added names for bool args at call sites - Use expression-bodied members in a few places - Pass lower bounds to Array.Copy - Remove unnecessary "success" local in QueueUserworkItemHelper Commit migrated from https://github.com/dotnet/coreclr/commit/ade717c7bbca40632db3b42885589909d645fc1a --- diff --git a/src/coreclr/src/mscorlib/src/System/Threading/ThreadPool.cs b/src/coreclr/src/mscorlib/src/System/Threading/ThreadPool.cs index 0645ce5..f56a082 100644 --- a/src/coreclr/src/mscorlib/src/System/Threading/ThreadPool.cs +++ b/src/coreclr/src/mscorlib/src/System/Threading/ThreadPool.cs @@ -11,37 +11,22 @@ ** =============================================================================*/ -#pragma warning disable 0420 - -/* - * Below you'll notice two sets of APIs that are separated by the - * use of 'Unsafe' in their names. The unsafe versions are called - * that because they do not propagate the calling stack onto the - * worker thread. This allows code to lose the calling stack and - * thereby elevate its security privileges. Note that this operation - * is much akin to the combined ability to control security policy - * and control security evidence. With these privileges, a person - * can gain the right to load assemblies that are fully trusted which - * then assert full trust and can call any code they want regardless - * of the previous stack information. - */ +using System.Security; +using System.Security.Permissions; +using System; +using Microsoft.Win32; +using System.Runtime.CompilerServices; +using System.Runtime.ConstrainedExecution; +using System.Runtime.InteropServices; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Diagnostics; +using System.Diagnostics.Contracts; +using System.Diagnostics.CodeAnalysis; +using System.Diagnostics.Tracing; namespace System.Threading { - using System.Security; - using System.Security.Permissions; - using System; - using Microsoft.Win32; - using System.Runtime.CompilerServices; - using System.Runtime.ConstrainedExecution; - using System.Runtime.InteropServices; - using System.Collections.Concurrent; - using System.Collections.Generic; - using System.Diagnostics; - using System.Diagnostics.Contracts; - using System.Diagnostics.CodeAnalysis; - using System.Diagnostics.Tracing; - internal static class ThreadPoolGlobals { //Per-appDomain quantum (in ms) for which the thread keeps processing @@ -61,7 +46,7 @@ namespace System.Threading internal sealed class ThreadPoolWorkQueue { // Simple sparsely populated array to allow lock-free reading. - internal class SparseArray where T : class + internal sealed class SparseArray where T : class { private volatile T[] m_array; @@ -70,10 +55,7 @@ namespace System.Threading m_array = new T[initialSize]; } - internal T[] Current - { - get { return m_array; } - } + internal T[] Current => m_array; internal int Add(T e) { @@ -96,7 +78,7 @@ namespace System.Threading continue; T[] newArray = new T[array.Length * 2]; - Array.Copy(array, newArray, i + 1); + Array.Copy(array, 0, newArray, 0, i + 1); newArray[i + 1] = e; m_array = newArray; return i + 1; @@ -123,7 +105,7 @@ namespace System.Threading } } - internal class WorkStealingQueue + internal sealed class WorkStealingQueue { private const int INITIAL_SIZE = 32; internal volatile IThreadPoolWorkItem[] m_array = new IThreadPoolWorkItem[INITIAL_SIZE]; @@ -139,7 +121,7 @@ namespace System.Threading private volatile int m_headIndex = START_INDEX; private volatile int m_tailIndex = START_INDEX; - private SpinLock m_foreignLock = new SpinLock(false); + private SpinLock m_foreignLock = new SpinLock(enableThreadOwnerTracking:false); public void LocalPush(IThreadPoolWorkItem obj) { @@ -173,7 +155,7 @@ namespace System.Threading finally { if (lockTaken) - m_foreignLock.Exit(true); + m_foreignLock.Exit(useMemoryBarrier:true); } } @@ -198,7 +180,7 @@ namespace System.Threading if (count >= m_mask) { // We're full; expand the queue by doubling its size. - IThreadPoolWorkItem[] newArray = new IThreadPoolWorkItem[m_array.Length << 1]; + var newArray = new IThreadPoolWorkItem[m_array.Length << 1]; for (int i = 0; i < m_array.Length; i++) newArray[i] = m_array[(i + head) & m_mask]; @@ -215,7 +197,7 @@ namespace System.Threading finally { if (lockTaken) - m_foreignLock.Exit(false); + m_foreignLock.Exit(useMemoryBarrier:false); } } } @@ -273,7 +255,7 @@ namespace System.Threading finally { if (lockTaken) - m_foreignLock.Exit(false); + m_foreignLock.Exit(useMemoryBarrier:false); } } } @@ -340,7 +322,7 @@ namespace System.Threading finally { if (lockTaken) - m_foreignLock.Exit(false); + m_foreignLock.Exit(useMemoryBarrier:false); } } } @@ -384,7 +366,7 @@ namespace System.Threading finally { if (taken) - m_foreignLock.Exit(false); + m_foreignLock.Exit(useMemoryBarrier:false); } missedSteal = true; @@ -405,12 +387,9 @@ namespace System.Threading loggingEnabled = FrameworkEventSource.Log.IsEnabled(EventLevel.Verbose, FrameworkEventSource.Keywords.ThreadPool|FrameworkEventSource.Keywords.ThreadTransfer); } - public ThreadPoolWorkQueueThreadLocals EnsureCurrentThreadHasQueue() - { - if (null == ThreadPoolWorkQueueThreadLocals.threadLocals) - ThreadPoolWorkQueueThreadLocals.threadLocals = new ThreadPoolWorkQueueThreadLocals(this); - return ThreadPoolWorkQueueThreadLocals.threadLocals; - } + public ThreadPoolWorkQueueThreadLocals EnsureCurrentThreadHasQueue() => + ThreadPoolWorkQueueThreadLocals.threadLocals ?? + (ThreadPoolWorkQueueThreadLocals.threadLocals = new ThreadPoolWorkQueueThreadLocals(this)); internal void EnsureThreadRequested() { @@ -454,12 +433,12 @@ namespace System.Threading public void Enqueue(IThreadPoolWorkItem callback, bool forceGlobal) { + if (loggingEnabled) + System.Diagnostics.Tracing.FrameworkEventSource.Log.ThreadPoolEnqueueWorkObject(callback); + ThreadPoolWorkQueueThreadLocals tl = null; if (!forceGlobal) tl = ThreadPoolWorkQueueThreadLocals.threadLocals; - - if (loggingEnabled) - System.Diagnostics.Tracing.FrameworkEventSource.Log.ThreadPoolEnqueueWorkObject(callback); if (null != tl) { @@ -511,7 +490,7 @@ namespace System.Threading return callback; } - static internal bool Dispatch() + internal static bool Dispatch() { var workQueue = ThreadPoolGlobals.workQueue; // @@ -604,7 +583,7 @@ namespace System.Threading try { } finally { - ThreadPool.ReportThreadStatus(true); + ThreadPool.ReportThreadStatus(isWorking:true); reportedStatus = true; } workItem.ExecuteWorkItem(); @@ -613,7 +592,7 @@ namespace System.Threading finally { if (reportedStatus) - ThreadPool.ReportThreadStatus(false); + ThreadPool.ReportThreadStatus(isWorking:false); } } else @@ -641,8 +620,7 @@ namespace System.Threading // it was executed or not (in debug builds only). Task uses this to communicate the ThreadAbortException to anyone // who waits for the task to complete. // - if (workItem != null) - workItem.MarkAborted(tae); + workItem?.MarkAborted(tae); // // In this case, the VM is going to request another thread on our behalf. No need to do it twice. @@ -661,7 +639,7 @@ namespace System.Threading } // we can never reach this point, but the C# compiler doesn't know that, because it doesn't know the ThreadAbortException will be reraised above. - Debug.Assert(false); + Debug.Fail("Should never reach this point"); return true; } } @@ -696,11 +674,11 @@ namespace System.Threading try { } finally { - IThreadPoolWorkItem cb = null; + IThreadPoolWorkItem cb; if (workStealingQueue.LocalPop(out cb)) { Debug.Assert(null != cb); - workQueue.Enqueue(cb, true); + workQueue.Enqueue(cb, forceGlobal:true); } else { @@ -728,27 +706,13 @@ namespace System.Threading internal sealed class RegisteredWaitHandleSafe : CriticalFinalizerObject { - private static IntPtr InvalidHandle - { - get - { - return Win32Native.INVALID_HANDLE_VALUE; - } - } - private IntPtr registeredWaitHandle; + private static IntPtr InvalidHandle => Win32Native.INVALID_HANDLE_VALUE; + private IntPtr registeredWaitHandle = InvalidHandle; private WaitHandle m_internalWaitObject; private bool bReleaseNeeded = false; private volatile int m_lock = 0; - internal RegisteredWaitHandleSafe() - { - registeredWaitHandle = InvalidHandle; - } - - internal IntPtr GetHandle() - { - return registeredWaitHandle; - } + internal IntPtr GetHandle() => registeredWaitHandle; internal void SetHandle(IntPtr handle) { @@ -825,10 +789,8 @@ namespace System.Threading return result; } - private bool ValidHandle() - { - return (registeredWaitHandle != InvalidHandle && registeredWaitHandle != IntPtr.Zero); - } + private bool ValidHandle() => + registeredWaitHandle != InvalidHandle && registeredWaitHandle != IntPtr.Zero; ~RegisteredWaitHandleSafe() { @@ -887,9 +849,9 @@ namespace System.Threading private static extern bool UnregisterWaitNative(IntPtr handle, SafeHandle waitObject); } -[System.Runtime.InteropServices.ComVisible(true)] + [System.Runtime.InteropServices.ComVisible(true)] public sealed class RegisteredWaitHandle : MarshalByRefObject { - private RegisteredWaitHandleSafe internalRegisteredWait; + private readonly RegisteredWaitHandleSafe internalRegisteredWait; internal RegisteredWaitHandle() { @@ -906,8 +868,7 @@ namespace System.Threading internalRegisteredWait.SetWaitObject(waitObject); } - -[System.Runtime.InteropServices.ComVisible(true)] + [System.Runtime.InteropServices.ComVisible(true)] // This is the only public method on this class public bool Unregister( WaitHandle waitObject // object to be notified when all callbacks to delegates have completed @@ -931,10 +892,7 @@ namespace System.Threading // internal static class _ThreadPoolWaitCallback { - static internal bool PerformWaitCallback() - { - return ThreadPoolWorkQueue.Dispatch(); - } + internal static bool PerformWaitCallback() => ThreadPoolWorkQueue.Dispatch(); } // @@ -955,8 +913,8 @@ namespace System.Threading internal sealed class QueueUserWorkItemCallback : IThreadPoolWorkItem { private WaitCallback callback; - private ExecutionContext context; - private Object state; + private readonly ExecutionContext context; + private readonly Object state; #if DEBUG volatile int executed; @@ -987,7 +945,7 @@ namespace System.Threading void IThreadPoolWorkItem.ExecuteWorkItem() { #if DEBUG - MarkExecuted(false); + MarkExecuted(aborted:false); #endif // call directly if it is an unsafe call OR EC flow is suppressed if (context == null) @@ -998,7 +956,7 @@ namespace System.Threading } else { - ExecutionContext.Run(context, ccb, this, true); + ExecutionContext.Run(context, ccb, this, preserveSyncCtx:true); } } @@ -1007,16 +965,16 @@ namespace System.Threading #if DEBUG // this workitem didn't execute because we got a ThreadAbortException prior to the call to ExecuteWorkItem. // This counts as being executed for our purposes. - MarkExecuted(true); + MarkExecuted(aborted:true); #endif } - static internal ContextCallback ccb = new ContextCallback(WaitCallback_Context); + internal static readonly ContextCallback ccb = new ContextCallback(WaitCallback_Context); - static private void WaitCallback_Context(Object state) + private static void WaitCallback_Context(Object state) { QueueUserWorkItemCallback obj = (QueueUserWorkItemCallback)state; - WaitCallback wc = obj.callback as WaitCallback; + WaitCallback wc = obj.callback; Debug.Assert(null != wc); wc(obj.state); } @@ -1025,7 +983,7 @@ namespace System.Threading internal sealed class QueueUserWorkItemCallbackDefaultContext : IThreadPoolWorkItem { private WaitCallback callback; - private Object state; + private readonly Object state; #if DEBUG private volatile int executed; @@ -1055,9 +1013,9 @@ namespace System.Threading void IThreadPoolWorkItem.ExecuteWorkItem() { #if DEBUG - MarkExecuted(false); + MarkExecuted(aborted:false); #endif - ExecutionContext.Run(ExecutionContext.PreAllocatedDefault, ccb, this, true); + ExecutionContext.Run(ExecutionContext.PreAllocatedDefault, ccb, this, preserveSyncCtx:true); } void IThreadPoolWorkItem.MarkAborted(ThreadAbortException tae) @@ -1065,16 +1023,16 @@ namespace System.Threading #if DEBUG // this workitem didn't execute because we got a ThreadAbortException prior to the call to ExecuteWorkItem. // This counts as being executed for our purposes. - MarkExecuted(true); + MarkExecuted(aborted:true); #endif } - static internal ContextCallback ccb = new ContextCallback(WaitCallback_Context); + internal static readonly ContextCallback ccb = new ContextCallback(WaitCallback_Context); - static private void WaitCallback_Context(Object state) + private static void WaitCallback_Context(Object state) { QueueUserWorkItemCallbackDefaultContext obj = (QueueUserWorkItemCallbackDefaultContext)state; - WaitCallback wc = obj.callback as WaitCallback; + WaitCallback wc = obj.callback; Debug.Assert(null != wc); obj.callback = null; wc(obj.state); @@ -1086,8 +1044,8 @@ namespace System.Threading WaitOrTimerCallback _waitOrTimerCallback; ExecutionContext _executionContext; Object _state; - static private ContextCallback _ccbt = new ContextCallback(WaitOrTimerCallback_Context_t); - static private ContextCallback _ccbf = new ContextCallback(WaitOrTimerCallback_Context_f); + private static readonly ContextCallback _ccbt = new ContextCallback(WaitOrTimerCallback_Context_t); + private static readonly ContextCallback _ccbf = new ContextCallback(WaitOrTimerCallback_Context_f); internal _ThreadPoolWaitOrTimerCallback(WaitOrTimerCallback waitOrTimerCallback, Object state, bool compressStack, ref StackCrawlMark stackMark) { @@ -1103,24 +1061,20 @@ namespace System.Threading } } - static private void WaitOrTimerCallback_Context_t(Object state) - { - WaitOrTimerCallback_Context(state, true); - } + private static void WaitOrTimerCallback_Context_t(Object state) => + WaitOrTimerCallback_Context(state, timedOut:true); - static private void WaitOrTimerCallback_Context_f(Object state) - { - WaitOrTimerCallback_Context(state, false); - } + private static void WaitOrTimerCallback_Context_f(Object state) => + WaitOrTimerCallback_Context(state, timedOut:false); - static private void WaitOrTimerCallback_Context(Object state, bool timedOut) + private static void WaitOrTimerCallback_Context(Object state, bool timedOut) { _ThreadPoolWaitOrTimerCallback helper = (_ThreadPoolWaitOrTimerCallback)state; helper._waitOrTimerCallback(helper._state, timedOut); } // call back helper - static internal void PerformWaitOrTimerCallback(Object state, bool timedOut) + internal static void PerformWaitOrTimerCallback(Object state, bool timedOut) { _ThreadPoolWaitOrTimerCallback helper = (_ThreadPoolWaitOrTimerCallback)state; Debug.Assert(helper != null, "Null state passed to PerformWaitOrTimerCallback!"); @@ -1134,10 +1088,7 @@ namespace System.Threading { using (ExecutionContext executionContext = helper._executionContext.CreateCopy()) { - if (timedOut) - ExecutionContext.Run(executionContext, _ccbt, helper, true); - else - ExecutionContext.Run(executionContext, _ccbf, helper, true); + ExecutionContext.Run(executionContext, timedOut ? _ccbt : _ccbf, helper, preserveSyncCtx:true); } } } @@ -1153,7 +1104,6 @@ namespace System.Threading public static class ThreadPool { - public static bool SetMaxThreads(int workerThreads, int completionPortThreads) { return SetMaxThreadsNative(workerThreads, completionPortThreads); @@ -1376,43 +1326,38 @@ namespace System.Threading //ThreadPool has per-appdomain managed queue of work-items. The VM is //responsible for just scheduling threads into appdomains. After that //work-items are dispatched from the managed queue. - private static bool QueueUserWorkItemHelper(WaitCallback callBack, Object state, ref StackCrawlMark stackMark, bool compressStack ) + private static bool QueueUserWorkItemHelper(WaitCallback callBack, Object state, ref StackCrawlMark stackMark, bool compressStack) { - bool success = true; - - if (callBack != null) + if (callBack == null) { - //The thread pool maintains a per-appdomain managed work queue. - //New thread pool entries are added in the managed queue. - //The VM is responsible for the actual growing/shrinking of - //threads. - - EnsureVMInitialized(); + throw new ArgumentNullException(nameof(WaitCallback)); + } - // - // If we are able to create the workitem, we need to get it in the queue without being interrupted - // by a ThreadAbortException. - // - try { } - finally - { - ExecutionContext context = compressStack && !ExecutionContext.IsFlowSuppressed() ? - ExecutionContext.Capture(ref stackMark, ExecutionContext.CaptureOptions.IgnoreSyncCtx | ExecutionContext.CaptureOptions.OptimizeDefaultCase) : - null; + //The thread pool maintains a per-appdomain managed work queue. + //New thread pool entries are added in the managed queue. + //The VM is responsible for the actual growing/shrinking of + //threads. - IThreadPoolWorkItem tpcallBack = context == ExecutionContext.PreAllocatedDefault ? - new QueueUserWorkItemCallbackDefaultContext(callBack, state) : - (IThreadPoolWorkItem)new QueueUserWorkItemCallback(callBack, state, context); + EnsureVMInitialized(); - ThreadPoolGlobals.workQueue.Enqueue(tpcallBack, true); - success = true; - } - } - else + // + // If we are able to create the workitem, we need to get it in the queue without being interrupted + // by a ThreadAbortException. + // + try { } + finally { - throw new ArgumentNullException(nameof(WaitCallback)); + ExecutionContext context = compressStack && !ExecutionContext.IsFlowSuppressed() ? + ExecutionContext.Capture(ref stackMark, ExecutionContext.CaptureOptions.IgnoreSyncCtx | ExecutionContext.CaptureOptions.OptimizeDefaultCase) : + null; + + IThreadPoolWorkItem tpcallBack = context == ExecutionContext.PreAllocatedDefault ? + new QueueUserWorkItemCallbackDefaultContext(callBack, state) : + (IThreadPoolWorkItem)new QueueUserWorkItemCallback(callBack, state, context); + + ThreadPoolGlobals.workQueue.Enqueue(tpcallBack, forceGlobal:true); } - return success; + return true; } internal static void UnsafeQueueCustomWorkItem(IThreadPoolWorkItem workItem, bool forceGlobal) @@ -1434,9 +1379,9 @@ namespace System.Threading internal static bool TryPopCustomWorkItem(IThreadPoolWorkItem workItem) { Debug.Assert(null != workItem); - if (!ThreadPoolGlobals.vmTpInitialized) - return false; //Not initialized, so there's no way this workitem was ever queued. - return ThreadPoolGlobals.workQueue.LocalFindAndPop(workItem); + return + ThreadPoolGlobals.vmTpInitialized && // if not initialized, so there's no way this workitem was ever queued. + ThreadPoolGlobals.workQueue.LocalFindAndPop(workItem); } // Get all workitems. Called by TaskScheduler in its debugger hooks. @@ -1505,20 +1450,14 @@ namespace System.Threading // This is the method the debugger will actually call, if it ends up calling // into ThreadPool directly. Tests can use this to simulate a debugger, as well. - internal static object[] GetQueuedWorkItemsForDebugger() - { - return ToObjectArray(GetQueuedWorkItems()); - } + internal static object[] GetQueuedWorkItemsForDebugger() => + ToObjectArray(GetQueuedWorkItems()); - internal static object[] GetGloballyQueuedWorkItemsForDebugger() - { - return ToObjectArray(GetGloballyQueuedWorkItems()); - } + internal static object[] GetGloballyQueuedWorkItemsForDebugger() => + ToObjectArray(GetGloballyQueuedWorkItems()); - internal static object[] GetLocallyQueuedWorkItemsForDebugger() - { - return ToObjectArray(GetLocallyQueuedWorkItems()); - } + internal static object[] GetLocallyQueuedWorkItemsForDebugger() => + ToObjectArray(GetLocallyQueuedWorkItems()); [DllImport(JitHelpers.QCall, CharSet = CharSet.Unicode)] [SuppressUnmanagedCodeSecurity] @@ -1528,10 +1467,8 @@ namespace System.Threading unsafe private static extern bool PostQueuedCompletionStatus(NativeOverlapped* overlapped); [CLSCompliant(false)] - unsafe public static bool UnsafeQueueNativeOverlapped(NativeOverlapped* overlapped) - { - return PostQueuedCompletionStatus(overlapped); - } + unsafe public static bool UnsafeQueueNativeOverlapped(NativeOverlapped* overlapped) => + PostQueuedCompletionStatus(overlapped); private static void EnsureVMInitialized() { @@ -1595,9 +1532,7 @@ namespace System.Threading [Obsolete("ThreadPool.BindHandle(IntPtr) has been deprecated. Please use ThreadPool.BindHandle(SafeHandle) instead.", false)] - public static bool BindHandle( - IntPtr osHandle - ) + public static bool BindHandle(IntPtr osHandle) { return BindIOCompletionCallbackNative(osHandle); }