bpo-30581: Windows: os.cpu_count() returns wrong number of processors #2934
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
…ount API on Windows calls to os.cpu_count()
crwilcox
force-pushed
the
bug/windows-processor-counts
branch
from
7e6a49c to
73adde2
Compare
| /* Vista is supported and the GetMaximumProcessorCount API is Win7+ | ||
| Need to fallback to Vista behavior if this call isn't present */ | ||
| HINSTANCE hKernel32; | ||
| hKernel32 = GetModuleHandleW(L"KERNEL32"); |
There was a problem hiding this comment.
Does the handle need to be closed after using it?
There was a problem hiding this comment.
Other places in posixmodule use this but on closer inspection they are all in methods.
There was a problem hiding this comment.
No, GetModuleHandle does not increase the refcount on the module (it also only works on already-loaded modules, but kernel32 is guaranteed to be there).
| *(FARPROC*)&_GetMaximumProcessorCount = GetProcAddress(hKernel32, | ||
| "GetMaximumProcessorCount"); | ||
| if (_GetMaximumProcessorCount != NULL) { | ||
| ncpu = _GetMaximumProcessorCount(ALL_PROCESSOR_GROUPS); |
There was a problem hiding this comment.
Is ALL_PROCESSOR_GROUPS always defined at compile-time, even before Windows 7?
There was a problem hiding this comment.
I think this may be just windows 7 forward. I know the result needs to run on Pre-Windows 7, does it also need to compile?
There was a problem hiding this comment.
In the Windows 10 SDK (which is required) the symbol is always defined. We filter APIs to those available on Vista, but that symbol is not excluded when you do that.
There was a problem hiding this comment.
This still requires a NEWS.d entry, and the documentation should be updated to indicate:
- the behaviour of the function has changed on Windows
- the fact that a 32-bit interpreter on 64-bit Windows will never return (nor have access to) more than 32 CPUs
I don't believe the latter point is a problem - the current behaviour of the API has the same issue. This falls into the same category as sys.getwindowsversion() reporting what the operating system provides, even when this is incorrect (e.g. Python 2.7 will always return 6.2 even on Windows 10, and the OS also shims certain APIs to behave like the previous version). Since the process is limited by the OS to only 32 CPUs, the os module API should reflect that limit.
|
Before merging please consider my comment here: |
|
@giampaolo, I had responded. Is there something I didn't address? |
|
@giampaolo, do you have more concerns here? |
|
Sorry for the delay (I've been backpacking / travelling through China). |
|
Thank you @crwilcox ! |
…python#2934) * Fixes python#30581 by adding a path to use newer GetMaximumProcessorCount API on Windows calls to os.cpu_count() * Add NEWS.d entry for bpo-30581, os.cpu_count on Windows. * Tweak NEWS entry
…python#2934) * Fixes python#30581 by adding a path to use newer GetMaximumProcessorCount API on Windows calls to os.cpu_count() * Add NEWS.d entry for bpo-30581, os.cpu_count on Windows. * Tweak NEWS entry
arhadthedev
changed the title
arhadthedev
changed the title
os.cpu_count() returns wrong number of processors on system with > 64 logical processors
https://bugs.python.org/issue30581