windows: provide more accurate result for available_parallelism if cores > 64 #114856

klensy · 2023-08-15T13:45:55Z

This should give more accurate result if number of cores > 64

see:
https://devblogs.microsoft.com/oldnewthing/20200824-00/?p=104116
ninja-build/ninja#1604

Could probably implement using GetLogicalProcessorInformationEx
see: ninja-build/ninja#1674

https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getactiveprocessorcount >= Win 7

crates that probably want to fix it too:
sysinfo: but maybe GetLogicalProcessorInformationEx instead?
num_cpus: obviously

rustbot · 2023-08-15T13:46:00Z

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

klensy · 2023-08-15T13:46:48Z

Anyone with > 64 cores test please?

ChrisDenton

Do the docs also need updating with Windows specific information?

library/std/src/sys/windows/thread.rs

klensy · 2023-08-15T14:44:27Z

Do the docs also need updating with Windows specific information?

https://doc.rust-lang.org/nightly/std/thread/fn.available_parallelism.html
Docs already mention that this can be not accurate, so i don't know.

It may undercount the amount of parallelism available on systems with more than 64 logical CPUs

rustbot · 2023-08-15T15:36:21Z

The Miri subtree was changed

cc @rust-lang/miri

klensy · 2023-08-15T15:37:25Z

src/tools/miri/src/shims/windows/foreign_items.rs

+            "GetActiveProcessorCount" => {
+                let [groupnumber] = this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?;
+                this.read_scalar(groupnumber)?.to_u16()?;
+                this.write_scalar(Scalar::from_u32(this.machine.num_cpus), dest)?;
+            }


This probably wrong, need advice.

cc @rust-lang/miri

We were already summoned by rustbot automatically :)

Why do you think this is wrong?

This should be further up the match, down here is the section for if this.frame_in_std() shims. Probably putting it with GetSystemInfo makes sense.

Other than that -- without knowing GetActiveProcessorCount, this seems reasonable? A brief comment explaining why we can ignore the groupnumber might be good.

Moved and added comment.

klensy · 2023-08-15T16:03:16Z

Uhh, can't this be more human-friendly message?
{"message":"the place in the program where the ICE was triggered",...

joshtriplett · 2023-08-15T16:13:26Z

Given that Windows 11 appears to lift the 64-CPU limitation by default without requiring any special handling from a process, this seems reasonable.

r=me once CI passes, and once someone from miri signs off on the miri changes.

joshtriplett · 2023-08-15T16:23:50Z

@rustbot author

RalfJung · 2023-08-15T16:46:39Z

Uhh, can't this be more human-friendly message? {"message":"the place in the program where the ICE was triggered",...

Yeah we're working on that. Or at least I hope this has the side-effect of making things better. I don't know why it uses JSON, it is some strange interaction of x.py test and the Miri test suite.

RalfJung · 2023-08-15T16:49:58Z

library/std/src/sys/windows/c/windows_sys.rs

@@ -3875,47 +3875,6 @@ pub const SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE: SYMBOLIC_LINK_FLAGS = 2u
 pub const SYMBOLIC_LINK_FLAG_DIRECTORY: SYMBOLIC_LINK_FLAGS = 1u32;
 pub const SYMLINK_FLAG_RELATIVE: u32 = 1u32;
 pub const SYNCHRONIZE: FILE_ACCESS_RIGHTS = 1048576u32;
-#[repr(C)]
-pub struct SYSTEM_INFO {


Miri was relying on this type declaration for its GetSystemInfo shim, that's why we are getting ICEs now.

The alternative is to change Miri's GetSystemInfo to use hard-coded offsets for the two fields it cares about (dwPageSize, dwNumberOfProcessors).

I think it's ok to keep the old definitions around even though they're no longer used by std. I don't think there's yet a pressing need to keep windows_sys.rs definitions strictly minimal,. Plus keeping them reduces churn should we decide to use them for something else in the future.

I can also disentangle Miri from that definition, if you'd prefer us not to rely on it. That can happen asynchronously with this PR, so eventually when the Miri patch lands you can do the cleanup.

I mean, it might be better overall if miri wasn't entangled with the std this way but I don't feel particularly strongly about it.

On Unix we rely on libc for tons of constants and types. That's just a lot more maintainable than hard-coding them all in the Miri sources. For Windows sadly there is no libc-equivalent (that is present in the sysroot) so we can only use the few things we can find in std...

Can you not use the windows-sys crate directly? The definitions in std are essentially just a vendored version of it.

We need a crate that's in the sysroot. (Miri might be running a Windows target on a Linux host.) So... we can do that once std depends on it. ;)

the8472 · 2023-08-15T18:29:41Z

Do the docs also need updating with Windows specific information?

https://doc.rust-lang.org/nightly/std/thread/fn.available_parallelism.html Docs already mention that this can be not accurate, so i don't know.

It may undercount the amount of parallelism available on systems with more than 64 logical CPUs

Yes, this should be updated if that restriction is lifted. Based on #114856 (comment) it should now mention that may be overreporting on windows 10 because processes won't use more than 64 cores by default.

…res > 64

RalfJung · 2023-08-16T12:00:22Z

src/tools/miri/src/shims/windows/foreign_items.rs

+            // Number of cores, but more accurate vaulue, than GetSystemInfo.dwNumberOfProcessors
+            // if number of cores > 64. If called with ALL_PROCESSOR_GROUPS as groupnumber,
+            // returns total number of cores, instead of number in group, see
+            // https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getactiveprocessorcount


This sounds like we should do throw_unsup_format! if the groupnumber is not ALL_PROCESSOR_GROUPS, since we are not correctly handling that case.

vaulue typo.

klensy · 2023-08-18T13:01:48Z

library/std/src/sys/windows/thread.rs

-        let mut sysinfo: c::SYSTEM_INFO = crate::mem::zeroed();
-        c::GetSystemInfo(&mut sysinfo);
-        sysinfo.dwNumberOfProcessors as usize
+        // FIXME: windows::Win32::System::SystemServices::ALL_PROCESSOR_GROUPS should be u16, not u32


microsoft/win32metadata@8f70180
will be changed to Windows.Win32.System.Threading.Apis.ALL_PROCESSOR_GROUPS

Hm... I've been thinking about this some more. Could you move the correct definition of ALL_PROCESSOR_GROUPS into c.rs? That way we can keep all the FIXMEs into one file so they're easier to find and fix when the bindings are next updated. Otherwise it might be easier to forget about it.

JohnCSimon · 2023-11-12T17:18:18Z

@klensy
ping from triage - can you post your status on this PR? There hasn't been an update in a few months. Thanks!

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

bors · 2023-11-24T11:46:34Z

☔ The latest upstream changes (presumably #118232) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon · 2023-12-17T21:20:07Z

@klensy
Ping from triage: I'm closing this due to inactivity because it hasn't been touched in months.
Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

rustbot assigned thomcc Aug 15, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 15, 2023

ChrisDenton reviewed Aug 15, 2023

View reviewed changes

library/std/src/sys/windows/thread.rs Outdated Show resolved Hide resolved

library/std/src/sys/windows/thread.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

klensy mentioned this pull request Aug 15, 2023

Bug: ALL_PROCESSOR_GROUPS is u32, while should be u16 microsoft/win32metadata#1680

Closed

klensy force-pushed the num-cpu branch from 900b1e2 to ea98f01 Compare August 15, 2023 14:34

This comment has been minimized.

Sign in to view

klensy commented Aug 15, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2023

RalfJung reviewed Aug 15, 2023

View reviewed changes

klensy added 2 commits August 16, 2023 14:31

windows: provide more accurate result for available_parallelism if co…

ebd39e5

…res > 64

miri: teach about GetActiveProcessorCount

4dfdc9d

klensy force-pushed the num-cpu branch from 6848fad to 4dfdc9d Compare August 16, 2023 11:31

RalfJung reviewed Aug 16, 2023

View reviewed changes

klensy commented Aug 18, 2023

View reviewed changes

JohnCSimon closed this Dec 17, 2023

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Dec 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

windows: provide more accurate result for available_parallelism if cores > 64 #114856

windows: provide more accurate result for available_parallelism if cores > 64 #114856

klensy commented Aug 15, 2023 •

edited

Loading

rustbot commented Aug 15, 2023

klensy commented Aug 15, 2023

ChrisDenton left a comment

This comment has been minimized.

klensy commented Aug 15, 2023

This comment has been minimized.

rustbot commented Aug 15, 2023

klensy Aug 15, 2023

joshtriplett Aug 15, 2023

saethlin Aug 15, 2023

RalfJung Aug 15, 2023

klensy Aug 16, 2023

This comment has been minimized.

klensy commented Aug 15, 2023

joshtriplett commented Aug 15, 2023

joshtriplett commented Aug 15, 2023

RalfJung commented Aug 15, 2023

RalfJung Aug 15, 2023

ChrisDenton Aug 15, 2023

RalfJung Aug 15, 2023

ChrisDenton Aug 15, 2023

RalfJung Aug 15, 2023 •

edited

Loading

ChrisDenton Aug 15, 2023

RalfJung Aug 15, 2023

the8472 commented Aug 15, 2023

RalfJung Aug 16, 2023

klensy Aug 16, 2023

klensy Aug 18, 2023

ChrisDenton Aug 18, 2023

JohnCSimon commented Nov 12, 2023

bors commented Nov 24, 2023

JohnCSimon commented Dec 17, 2023

windows: provide more accurate result for available_parallelism if cores > 64 #114856

windows: provide more accurate result for available_parallelism if cores > 64 #114856

Conversation

klensy commented Aug 15, 2023 • edited Loading

rustbot commented Aug 15, 2023

klensy commented Aug 15, 2023

ChrisDenton left a comment

Choose a reason for hiding this comment

This comment has been minimized.

klensy commented Aug 15, 2023

This comment has been minimized.

rustbot commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

klensy commented Aug 15, 2023

joshtriplett commented Aug 15, 2023

joshtriplett commented Aug 15, 2023

RalfJung commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the8472 commented Aug 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnCSimon commented Nov 12, 2023

bors commented Nov 24, 2023

JohnCSimon commented Dec 17, 2023

klensy commented Aug 15, 2023 •

edited

Loading

RalfJung Aug 15, 2023 •

edited

Loading