Remove ThreadPool native implementation #71719

eduardo-vp · 2022-07-06T16:25:23Z

Cleaning up the thread pool native implementation, removing ThreadpoolMgr::UsePortableThreadPool() and ThreadpoolMgr::UsePortableThreadPoolForIO() and assume the calls to those functions to always return true.

Deleting all the functions and code that became unused.

ghost · 2022-07-06T16:25:39Z

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author:	eduardo-vp
Assignees:	eduardo-vp
Labels:	`area-System.Threading`
Milestone:	-

davidwrighton

I like this. I see a couple of functions called by the AppDomain class which now look unnecessary, but other than that everything looks pretty good to me.

src/coreclr/vm/threadpoolrequest.cpp

src/coreclr/vm/hillclimbing.cpp

src/coreclr/vm/win32threadpool.cpp

…uff in dacvars.h

src/coreclr/inc/sospriv.idl

src/coreclr/debug/daccess/request.cpp

This reverts commit 9cf256c.

…priv.h" This reverts commit 8064d92.

src/coreclr/debug/daccess/request.cpp

src/coreclr/System.Private.CoreLib/src/System/Threading/Overlapped.cs

src/coreclr/vm/win32threadpool.cpp

src/coreclr/System.Private.CoreLib/src/System/Threading/ThreadPool.CoreCLR.cs

…Pool.CoreCLR.cs Co-authored-by: Stephen Toub <[email protected]>

teo-tsirpanis · 2022-07-10T10:42:27Z

src/coreclr/System.Private.CoreLib/src/System/Threading/Overlapped.cs

        internal static extern void FreeNativeOverlapped(NativeOverlapped* nativeOverlappedPtr);

        [MethodImpl(MethodImplOptions.InternalCall)]
        internal static extern OverlappedData GetOverlappedFromNative(NativeOverlapped* nativeOverlappedPtr);


How about porting the rest of the Overlapped fcalls to managed code? Especially GetOverlappedFromNative whose implementation is trivial (just casting a pointer and a couple of dereferences) would be benefitted from being inlined by the JIT.

Yes, there are more follow up improvements that can be done here. Not as part of this PR through.

deeprobin · 2022-07-11T04:47:59Z

I'm currently not that familiar with the internal ThreadPool APIs.

But it is a good step into the future that this PR will be merged and the ThreadPool stuff will be converted into managed code.

Will we get this merged before the .NET 7 release?

Just out of interest: Could you maybe provide some benchmarks? Just to make sure there are no major regressions.

deeprobin · 2022-07-11T04:51:31Z

src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.cs


+#if CORECLR
+#pragma warning disable CA1823
+        private static readonly bool s_initialized = ThreadPool.EnsureConfigInitialized();


What is the reason to use an unused field here?
Where is this used?

This is used to ensure that the threadpool config is initialized. It is the minimal change to preserve the existing initialization flow.

As mentioned above, there are more cleanups that can be done here, not part of this PR.

Okay. I was thinking of a static constructor, but of course it works that way too.

Adding regular static constructor would add static constructor triggers to all static methods that may introduce performance regressions.

src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.cs

src/coreclr/debug/daccess/request.cpp

jkotas · 2022-07-11T05:00:07Z

Just out of interest: Could you maybe provide some benchmarks? Just to make sure there are no major regressions.

@deeprobin This is result of several years long effort. It was benchmarked extensively along the way, see for example #64834 , #71864 , #38225 .

kouvel · 2022-07-12T04:25:03Z

Marked for .NET 8 for now. The full switchover to the portable thread pool has only completed in .NET 7 and the intention was to leave the fallback switches in place for .NET 7.

kouvel

Just a few comments, otherwise LGTM, thanks!

kouvel · 2022-08-11T12:16:38Z

src/coreclr/debug/daccess/request.cpp

-
-    SOSDacLeave();
-    return hr;
+    return E_NOTIMPL;


The SOS ThreadPool command would not work after this, it would need to be updated such that it continues to show info from the portable thread pool when receiving an E_NOTIMPL from this API, while keeping the legacy paths for backward compatibility.

Submitted: dotnet/diagnostics#3303

src/coreclr/System.Private.CoreLib/src/System/Threading/ThreadPool.CoreCLR.cs

src/coreclr/vm/ecalllist.h

src/coreclr/vm/threads.h

src/coreclr/vm/win32threadpool.cpp

deeprobin

What I can judge: LGTM.
I am looking forward to the change 👍🏼.

Remove UsePortableThreadPool() and UsePortableThreadPoolForIO()

d064de8

ghost assigned eduardo-vp Jul 6, 2022

ghost added the area-System.Threading label Jul 6, 2022

davidwrighton reviewed Jul 6, 2022

View reviewed changes

src/coreclr/vm/threadpoolrequest.cpp Outdated Show resolved Hide resolved

src/coreclr/vm/threadpoolrequest.cpp Outdated Show resolved Hide resolved

jkotas reviewed Jul 6, 2022

View reviewed changes

src/coreclr/vm/hillclimbing.cpp Show resolved Hide resolved

jkotas reviewed Jul 6, 2022

View reviewed changes

src/coreclr/vm/win32threadpool.cpp Outdated Show resolved Hide resolved

Eduardo Manuel Velarde Polar added 9 commits July 6, 2022 20:25

Delete more unnecessary functions

be8de40

Delete win32threadpool.cpp and win32threadpool.h

14e1757

Add win32threadpool with other changes

a5e1c9e

Add win32threadpool header back

a2b65d1

Current changes go until 381/577 during build native

aa50140

Delete GetHillClimbingLogEntry in request.cpp, Delete hillclimbing st…

568477e

…uff in dacvars.h

Delete hillclimbing stuff in function GetThreadpoolData in request.cpp

be9e5d4

Delete GetHillClimbingLogEntry in dacimpl.h, sospriv.idl, sospriv.h

8064d92

Delete DacpHillClimbingLogEntry in dacprivate.h

9cf256c

jkotas reviewed Jul 9, 2022

View reviewed changes

src/coreclr/inc/sospriv.idl Show resolved Hide resolved

jkotas reviewed Jul 9, 2022

View reviewed changes

src/coreclr/debug/daccess/request.cpp Show resolved Hide resolved

Eduardo Manuel Velarde Polar added 4 commits July 8, 2022 17:23

Revert "Delete DacpHillClimbingLogEntry in dacprivate.h"

d24f554

This reverts commit 9cf256c.

Revert "Delete GetHillClimbingLogEntry in dacimpl.h, sospriv.idl, sos…

c3a8389

…priv.h" This reverts commit 8064d92.

Change GetThreadpoolData in request.cpp to return E_NOTIMPL

566b4d7

Keep SOSDacLeave(); in GetThreadpoolData in request.cpp

ed44b47

jkotas reviewed Jul 9, 2022

View reviewed changes

src/coreclr/debug/daccess/request.cpp Show resolved Hide resolved

jkotas reviewed Jul 9, 2022

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Threading/Overlapped.cs Outdated Show resolved Hide resolved

jkotas reviewed Jul 9, 2022

View reviewed changes

src/coreclr/vm/win32threadpool.cpp Outdated Show resolved Hide resolved

jkotas added 3 commits July 9, 2022 08:11

Merge branch 'main' into update/remove-threadpool-native-implementation

b6291d5

Delete mode unused code

bdb28c7

Delete TPIndex

98fe65e

jkotas approved these changes Jul 10, 2022

View reviewed changes

stephentoub reviewed Jul 10, 2022

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Threading/ThreadPool.CoreCLR.cs Outdated Show resolved Hide resolved

Update src/coreclr/System.Private.CoreLib/src/System/Threading/Thread…

43efcf4

…Pool.CoreCLR.cs Co-authored-by: Stephen Toub <[email protected]>

stephentoub marked this pull request as ready for review July 10, 2022 01:18

stephentoub requested review from MichalStrehovsky, lambdageek, lateralusX and marek-safar as code owners July 10, 2022 01:18

stephentoub approved these changes Jul 10, 2022

View reviewed changes

teo-tsirpanis reviewed Jul 10, 2022

View reviewed changes

deeprobin reviewed Jul 11, 2022

View reviewed changes

kouvel added this to the 8.0.0 milestone Jul 12, 2022

stephentoub added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 12, 2022

kouvel mentioned this pull request Aug 12, 2022

Tracking issue for threading work items #52701

Open

17 tasks

kouvel approved these changes Aug 12, 2022

View reviewed changes

kouvel reviewed Aug 12, 2022

View reviewed changes

src/coreclr/vm/win32threadpool.cpp Show resolved Hide resolved

jkotas added 2 commits August 20, 2022 17:58

Merge branch 'main' into update/remove-threadpool-native-implementation

86aa5ac

CR feedback

afdd6c8

jkotas mentioned this pull request Aug 20, 2022

Update SOS for removal of unmanaged threadpool dotnet/diagnostics#3303

Merged

jkotas removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 20, 2022

deeprobin approved these changes Aug 20, 2022

View reviewed changes

jkotas added 2 commits August 20, 2022 19:53

Fix crst ordering`

e0ba959

Cleanup

11da1f5

jkotas merged commit bcd44bf into dotnet:main Aug 21, 2022

adityamandaleeka mentioned this pull request Aug 22, 2022

Remove the double dispatch on Windows for IO dotnet/aspnetcore#43449

Merged

ghost locked as resolved and limited conversation to collaborators Sep 20, 2022

Remove ThreadPool native implementation #71719

Remove ThreadPool native implementation #71719

Uh oh!

Conversation

eduardo-vp commented Jul 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 6, 2022

Uh oh!

davidwrighton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deeprobin commented Jul 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkotas commented Jul 11, 2022

Uh oh!

kouvel commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kouvel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deeprobin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

eduardo-vp commented Jul 6, 2022 •

edited

Loading

kouvel commented Jul 12, 2022 •

edited

Loading