Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Threadpool autorelease #47592

Merged
merged 23 commits into from
Feb 24, 2021
Merged

Threadpool autorelease #47592

merged 23 commits into from
Feb 24, 2021

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Jan 28, 2021 •
edited
Loading

Implement feature switch to set up NSAutoreleasePools on threadpool threads for each job dispatch.

@jkoritzinsky jkoritzinsky added this to the 6.0.0 milestone Jan 28, 2021
AaronRobinsonMSFT
AaronRobinsonMSFT reviewed Jan 28, 2021
View reviewed changes
@jkoritzinsky jkoritzinsky force-pushed the threadpool-autorelease branch from 293edea to 31b2bea Compare January 29, 2021 00:01
@jkoritzinsky jkoritzinsky changed the title Threadpool autorelease and MacOS Version info Threadpool autorelease Jan 29, 2021
@jkoritzinsky jkoritzinsky mentioned this pull request Jan 29, 2021
Merged
@jkoritzinsky jkoritzinsky mentioned this pull request Feb 4, 2021
Closed
@jkoritzinsky
Copy link
Member Author

Timeouts are #47850

Can I get another review pass.

@eerhardt can you validate that I set up the feature switch correctly?

eerhardt
eerhardt reviewed Feb 5, 2021
View reviewed changes
src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml Outdated
@@ -21,5 +21,8 @@
<type fullname="System.StartupHookProvider" feature="System.StartupHookProvider.IsSupported" featurevalue="false">
<method signature="System.Boolean get_IsSupported()" body="stub" value="false" />
</type>
<type fullname="System.Threading.ThreadPool" feature="System.Threading.ThreadPool.EnableDispatchAutoreleasePool" featurevalue="false">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be the other way. false is the default in the code. The feature switch is usually the opposite of the default. See GlobalizationMode.Invariant above. The default in the code is false and setting the feature switch to true trims away ICU dependencies.

Which value for this setting trims code? true or false?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting this switch to false (which is the default in code) should trim out the call to DispatchItemWithAutoreleasePool.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Just know that the code won't be trimmed unless someone explicitly sets the switch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the feature switch is now set up correctly.

I'm wondering how much code is actually going to be trimmed. It looks like only the ThreadPoolWorkQueue.DispatchItemWithAutoreleasePool static method, and 2 extern methods will be able to be trimmed. I still question if this is worth the effort of defining and maintaining a feature switch here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thread pool's performance is very sensitive to inlining and branching decisions. By enabling trimming here, we can make some of these branches disappear and eek out a little more performance. This one isn't particularly focused on size reduction.

eerhardt
eerhardt reviewed Feb 5, 2021
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Portable.cs Outdated
@@ -31,6 +31,9 @@ public static partial class ThreadPool
AppContextConfigHelper.GetBooleanConfig("System.Threading.ThreadPool.EnableWorkerTracking", false);
#endif

internal static bool EnableDispatchAutoreleasePool { get; } =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a partial class. Isn't there a file where you can put this property so it doesn't need to be duplicated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There actually isn't. ThreadPool.cs didn't actually have the ThreadPool type and I don't want to make a new ThreadPool.cs in the same PR where I'm renaming the old one because once it's squashed, the git history will get all messed up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put it in ThreadPoolWorkQueue.AutoreleasePool.OSX.cs?

public static partial class ThreadPool
{
    internal static bool EnableDispatchAutoreleasePool { get; } = AppContextConfigHelper.GetBooleanConfig("System.Threading.ThreadPool.EnableDispatchAutoreleasePool", false);
}

I understand that it is a minor violation of coding conventions, but I think it is acceptable.

Also, we won't be unnecessarily reading the property on non-OSX platforms anymore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@jkoritzinsky jkoritzinsky force-pushed the threadpool-autorelease branch from 8a94fde to c803877 Compare February 8, 2021 23:04
@jkoritzinsky
Copy link
Member Author

Anyone have any more feedback here before we merge this in?

@AaronRobinsonMSFT
Copy link
Member

@jkoritzinsky I think @stephentoub's request at #47592 (comment) is the only outstanding question I have.

@jkoritzinsky
Copy link
Member Author

Ok. Should I just run the benchmarks locally or is there a way I can run them on the benchmark lab machines for more reliable comparisons? Given that this is the threadpool and the possible perf change is extremely small, I don't know if I trust my personal machine to give stable enough results.

@jkoritzinsky
Move EnableDispatchAutoreleasePool definition to be local to the Disp…
a46dd70 
…atchItemWithAutoreleasePool definition file and remove the unused "unspported" impl.
@jkoritzinsky
Merge branch 'threadpool-autorelease' of github.com:jkoritzinsky/runt…
0fe7547 
…ime into threadpool-autorelease
@jkoritzinsky
Copy link
Member Author

iOS test run didn't time out on the second run, so I'll merge this in.

@jkoritzinsky jkoritzinsky merged commit 86eacff into dotnet:master Feb 24, 2021
@jkoritzinsky jkoritzinsky deleted the threadpool-autorelease branch February 24, 2021 01:19
@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rolfbjarne rolfbjarne rolfbjarne approved these changes

@marek-safar marek-safar marek-safar left review comments

@lambdageek lambdageek lambdageek left review comments

@stephentoub stephentoub stephentoub left review comments

@eerhardt eerhardt eerhardt left review comments

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT left review comments

@elinor-fung elinor-fung elinor-fung left review comments

@filipnavara filipnavara filipnavara approved these changes

@jkotas jkotas jkotas approved these changes

@kouvel kouvel Awaiting requested review from kouvel

Assignees
No one assigned
Labels
area-System.Runtime.InteropServices
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
10 participants
@jkoritzinsky @jkotas @AaronRobinsonMSFT @rolfbjarne @marek-safar @lambdageek @filipnavara @stephentoub @eerhardt @elinor-fung