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.

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

Threadpool autorelease #47592

Merged
merged 23 commits into from
Feb 24, 2021
Merged

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Jan 28, 2021

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

src/coreclr/interop/inc/interoplib.h Outdated Show resolved Hide resolved
src/coreclr/vm/interoplibinterface.h Outdated Show resolved Hide resolved
@jkoritzinsky jkoritzinsky changed the title Threadpool autorelease and MacOS Version info Threadpool autorelease Jan 29, 2021
@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?

@@ -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.

@@ -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
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.

…atchItemWithAutoreleasePool definition file and remove the unused "unspported" impl.
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants