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

Add boolean field to indicate whether or not the Windows thread pool is being used #90551

Merged
merged 6 commits into from
Aug 25, 2023

Conversation

eduardo-vp
Copy link
Member

@eduardo-vp eduardo-vp commented Aug 14, 2023

This field is being added for diagnostics purposes. It will be used by ThreadPool SOS command.

@ghost
Copy link

ghost commented Aug 14, 2023

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: -
Labels:

area-System.Threading

Milestone: -

Comment on lines 13 to 17
internal static bool UseWindowsThreadPool { get; } =
AppContextConfigHelper.GetBooleanConfig("System.Threading.ThreadPool.UseWindowsThreadPool", "DOTNET_ThreadPool_UseWindowsThreadPool");

// Exposes whether or not the Windows thread pool is used for diagnostics purposes
private static readonly bool s_useWindowsThreadPoolForDebugger = UseWindowsThreadPool;
Copy link
Member

@stephentoub stephentoub Aug 14, 2023

Choose a reason for hiding this comment

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

Suggested change
internal static bool UseWindowsThreadPool { get; } =
AppContextConfigHelper.GetBooleanConfig("System.Threading.ThreadPool.UseWindowsThreadPool", "DOTNET_ThreadPool_UseWindowsThreadPool");
// Exposes whether or not the Windows thread pool is used for diagnostics purposes
private static readonly bool s_useWindowsThreadPoolForDebugger = UseWindowsThreadPool;
private static readonly bool s_useWindowsThreadPool = // name relied on by sos
AppContextConfigHelper.GetBooleanConfig("System.Threading.ThreadPool.UseWindowsThreadPool", "DOTNET_ThreadPool_UseWindowsThreadPool");
internal static bool UseWindowsThreadPool => s_useWindowsThreadPool;

?

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 field should reflect what the property returns because the property can be stubbed by trimming, such that sos reflects the actual state of which thread pool is being used and not just the config value.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if coreclr can use trimming currently, and maybe it's not relevant currently, but I suppose it doesn't hurt to assume that either coreclr could use trimming in the future or an sos for NativeAOT could exist in the future and show similar info.

…adPool.Windows.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>
Copy link
Member

@kouvel kouvel left a comment

Choose a reason for hiding this comment

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

Just one comment, otherwise LGTM, thanks!

Copy link
Member

@kouvel kouvel left a comment

Choose a reason for hiding this comment

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

Actually sorry, I think what you had initially was more correct. The field should reflect what the property returns because the property can be stubbed by trimming, such that sos reflects the actual state of what thread pool is being used and not just the config value.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 22, 2023
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 22, 2023
Copy link
Member

@kouvel kouvel left a comment

Choose a reason for hiding this comment

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

Thanks!

@eduardo-vp eduardo-vp merged commit ea20228 into dotnet:main Aug 25, 2023
166 of 170 checks passed
@eduardo-vp
Copy link
Member Author

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6006105024

@ghost ghost locked as resolved and limited conversation to collaborators Sep 28, 2023
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.

3 participants