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

Update Environment.ProcessorCount on Windows to take into account the processor affinity mask #45943

Merged
merged 1 commit into from
Jan 25, 2021

Conversation

kouvel
Copy link
Member

@kouvel kouvel commented Dec 11, 2020

  • Similarly to cases on Unixes where sched_getaffinity is available
  • If GCCpuGroup and Thread_UseAllCpuGroups are both enabled, I'm not sure if the CPUGroupInfo count of active processors takes affinity into account as the docs are not clear, for now I'm not modifying that path until I can verify it
  • Otherwise, a process that is started with a specific processor affinity mask still shows full CPU count and perf is much worse in CPU-affinitized cases compared to the native thread pool
  • This is one of the differences in the portable managed thread pool implementation, which relies on Environment.ProcessorCount, as opposed to the native thread pool, which uses the affinity mask
  • After this change, in affinitized cases on Windows, perf-wise where this difference matters, the behavior perf-wise is closer to that on Linux and closer to what is currently expected:
    • The portable thread pool uses the same worker thread count as the native thread pool
    • Environment.ProcessorCount returns the number of processors the process is affinitized to, which may be less than it would have returned before, similarly to Linux

Breaking change issue: #47427

… processor affinity mask

- Similarly to cases on Unixes where sched_getaffinity is available
- If `GCCpuGroup` and `Thread_UseAllCpuGroups` are both enabled, I'm not sure if the `CPUGroupInfo` count of active processors takes affinity into account as the docs are not clear, for now I'm not modifying that path until I can verify it
- Otherwise, a process that is started with a specific processor affinity mask still shows full CPU count
- This is one of the differences in the portable managed thread pool implementation, which relies on Environment.ProcessorCount, as opposed to the native thread pool, which uses the affinity mask
- After this change, in affinitized cases on Windows the behavior is consistent perf-wise with Linux in similar situations:
  - The portable thread pool uses the same worker thread count as the native thread pool
  - `Environment.ProcessorCount` returns the number of processors the the process is affinitized to, which may be less than it would have returned before
@kouvel kouvel added this to the 6.0.0 milestone Dec 11, 2020
@kouvel kouvel requested a review from janvorli December 11, 2020 03:33
@kouvel kouvel self-assigned this Dec 11, 2020
@pgrawehr
Copy link

I don't think this is a good idea. After this change, how would one obtain the processor count, for instance to check whether an affinity mask is applied? For instance, I have a piece of code that uses this property and compares it to the affinity mask to make sure the application is allowed to run on all CPUs.

@jkotas
Copy link
Member

jkotas commented Dec 17, 2020

@pgrawehr The number of processors that the application can run on can be limited by number of mechanism. Process affinity, job control on Windows and cgroups on Linux are the most common mechanisms. We have found through trial-and-failure that returning the restricted number of processors from this property is the most appropriate behavior that makes most callers work well, and it is what we do on non-Windows systems already.

After this change, how would one obtain the processor count

You can PInvoke the appropriate OS APIs.

@kouvel I agree that this is a good change to make, but it needs to be marked as breaking change.

@pgrawehr
Copy link

You can PInvoke the appropriate OS APIs.

I think that's bad, because it's again OS dependent. The properties and methods in the Environment class are just there so the programmer doesn't need to use any low-level PInvoke calls for common system properties. Maybe adding a separate property would be nicer.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@danmoseley
Copy link
Member

@pgrawehr you could make an API proposal, with the template.

@danmoseley danmoseley added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Dec 17, 2020
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Dec 17, 2020
@stephentoub
Copy link
Member

@kouvel, can this be merged?

@kouvel
Copy link
Member Author

kouvel commented Jan 9, 2021

I just need to file an issue and document some things for the breaking change process, will try to do that early next week.

@stephentoub
Copy link
Member

Ok, cool.

@danmoseley
Copy link
Member

@kouvel when you open that breaking change issue please remove the "needs breaking change doc" label

@kouvel kouvel removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jan 25, 2021
jkotas added a commit to jkotas/runtimelab that referenced this pull request Jan 27, 2021
…he processor affinity mask

Port of dotnet/runtime#45943

Delete some dead code
jkotas added a commit to jkotas/runtimelab that referenced this pull request Jan 27, 2021
…he processor affinity mask

Port of dotnet/runtime#45943

Delete some dead code
jkotas added a commit to jkotas/runtimelab that referenced this pull request Jan 27, 2021
…he processor affinity mask

Port of dotnet/runtime#45943

Delete some dead code
jkotas added a commit to jkotas/runtimelab that referenced this pull request Jan 27, 2021
…he processor affinity mask

Port of dotnet/runtime#45943

Delete some dead code
MichalStrehovsky pushed a commit to dotnet/runtimelab that referenced this pull request Jan 28, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Threading breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants