-
Notifications
You must be signed in to change notification settings - Fork 503
WIP: Move portable thread pool and timer implementation to shared partition #6880
Changes from all commits
109ccbb
7d7a806
2a4710f
4db63c7
ae451d5
7eb9487
776178c
1a66e33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,16 +16,23 @@ internal partial class ClrThreadPool | |
| private WaitThreadNode _waitThreadsHead; | ||
| private WaitThreadNode _waitThreadsTail; | ||
|
|
||
| private LowLevelLock _waitThreadLock = new LowLevelLock(); | ||
| #if CORERT | ||
| private readonly Lock _waitThreadLock = new Lock(); | ||
| #else | ||
| private object _waitThreadLock = new object(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can create
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered it as an option. It would likely result in some counter-part like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm specifically going to evaluate the performance of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implementation-wise,
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The motivation behind this change was two-fold:
The problem is that it violates the condition that it is non-interruptible. Another problem is that I grossly underestimated the dependencies of |
||
| #endif | ||
|
|
||
| /// <summary> | ||
| /// Register a wait handle on a <see cref="WaitThread"/>. | ||
| /// </summary> | ||
| /// <param name="handle">A description of the requested registration.</param> | ||
| internal void RegisterWaitHandle(RegisteredWaitHandle handle) | ||
| { | ||
| _waitThreadLock.Acquire(); | ||
| try | ||
| #if CORERT | ||
| using (LockHolder.Hold(_waitThreadLock)) | ||
| #else | ||
| lock (_waitThreadLock) | ||
| #endif | ||
| { | ||
| if (_waitThreadsHead == null) // Lazily create the first wait thread. | ||
| { | ||
|
|
@@ -56,10 +63,6 @@ internal void RegisterWaitHandle(RegisteredWaitHandle handle) | |
| prev.Next.Thread.RegisterWaitHandle(handle); | ||
| return; | ||
| } | ||
| finally | ||
| { | ||
| _waitThreadLock.Release(); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -69,19 +72,18 @@ internal void RegisterWaitHandle(RegisteredWaitHandle handle) | |
| /// <returns><c>true</c> if the thread was successfully removed; otherwise, <c>false</c></returns> | ||
| private bool TryRemoveWaitThread(WaitThread thread) | ||
| { | ||
| _waitThreadLock.Acquire(); | ||
| try | ||
| #if CORERT | ||
| using (LockHolder.Hold(_waitThreadLock)) | ||
| #else | ||
| lock (_waitThreadLock) | ||
| #endif | ||
| { | ||
| if (thread.AnyUserWaits) | ||
| { | ||
| return false; | ||
| } | ||
| RemoveWaitThread(thread); | ||
| } | ||
| finally | ||
| { | ||
| _waitThreadLock.Release(); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
|
|
@@ -267,8 +269,11 @@ private void WaitThreadStart() | |
| /// </summary> | ||
| private void ProcessRemovals() | ||
| { | ||
| ThreadPoolInstance._waitThreadLock.Acquire(); | ||
| try | ||
| #if CORERT | ||
| using (LockHolder.Hold(ThreadPoolInstance._waitThreadLock)) | ||
| #else | ||
| lock (ThreadPoolInstance._waitThreadLock) | ||
| #endif | ||
| { | ||
| Debug.Assert(_numPendingRemoves >= 0); | ||
| Debug.Assert(_numPendingRemoves <= _pendingRemoves.Length); | ||
|
|
@@ -307,10 +312,6 @@ private void ProcessRemovals() | |
| Debug.Assert(originalNumUserWaits - originalNumPendingRemoves == _numUserWaits, | ||
| $"{originalNumUserWaits} - {originalNumPendingRemoves} == {_numUserWaits}"); | ||
| } | ||
| finally | ||
| { | ||
| ThreadPoolInstance._waitThreadLock.Release(); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -350,7 +351,6 @@ private void CompleteWait(object state) | |
| /// <returns>If the handle was successfully registered on this wait thread.</returns> | ||
| public bool RegisterWaitHandle(RegisteredWaitHandle handle) | ||
| { | ||
| ThreadPoolInstance._waitThreadLock.VerifyIsLocked(); | ||
| if (_numUserWaits == WaitHandle.MaxWaitHandles - 1) | ||
| { | ||
| return false; | ||
|
|
@@ -389,8 +389,11 @@ private void UnregisterWait(RegisteredWaitHandle handle, bool blocking) | |
| { | ||
| bool pendingRemoval = false; | ||
| // TODO: Optimization: Try to unregister wait directly if it isn't being waited on. | ||
| ThreadPoolInstance._waitThreadLock.Acquire(); | ||
| try | ||
| #if CORERT | ||
| using (LockHolder.Hold(ThreadPoolInstance._waitThreadLock)) | ||
| #else | ||
| lock (ThreadPoolInstance._waitThreadLock) | ||
| #endif | ||
| { | ||
| // If this handle is not already pending removal and hasn't already been removed | ||
| if (Array.IndexOf(_registeredWaits, handle) != -1 && Array.IndexOf(_pendingRemoves, handle) == -1) | ||
|
|
@@ -400,10 +403,6 @@ private void UnregisterWait(RegisteredWaitHandle handle, bool blocking) | |
| pendingRemoval = true; | ||
| } | ||
| } | ||
| finally | ||
| { | ||
| ThreadPoolInstance._waitThreadLock.Release(); | ||
| } | ||
|
|
||
| if (blocking) | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Little bit confusing naming here, is ClrThreadPool equal to ThreadPool.Portable ?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept the original naming of the files. There's
ThreadPool.Portable.cswhich implements theThreadPoolmethods. The other files are the internal implementation classes, which are created and called from theThreadPool.Portable.cscode.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the naming comes from an implementation inside CLR vs. Win32 Thread Pool API (https://docs.microsoft.com/en-us/windows/desktop/procthread/thread-pool-api).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to follow the existing naming convention. ThreadPool.cs for portable code, ThreadPool.CoreCLR.cs for CoreCLR runtime specific implementation
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already
ThreadPool.csshared between all implementations. Then there are three implementations of the actual thread pool (CoreCLR w/ unmanaged code and PAL, CortRT/Portable, CoreRT/Windows).This moves one of the implementations (CoreRT/Portable) under feature flag to shared partition. This implementation is managed reimplementation of what CoreCLR does and it's currently used by CoreRT on Unix.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to use the CoreRT implementation in Mono by simply adding
to Mono's System.Private.CoreLib.csproj. It could eventually be used in CoreCLR too, but that will require a lot of performance testing and changes to unmanaged code that are currently not feasible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it helps, it would be ok to create PortableThreadPool subdirectory and move the portable threadpool implementation there.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And rename
ClrThreadPool.*.cstoPortableThreadPool.*.cs, presumably, or something along those lines?