Skip to content

Enable ninja to use > 64 CPUs on Windows #1604

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

Merged
merged 2 commits into from
Aug 8, 2019

Conversation

jessemckenna
Copy link
Contributor

This incorporates a fix to ninja's calculation of the number of processors on Windows systems with more than 64 cores.

Windows systems with more than 64 logical processors divide the processors into groups, with most applications using only one group. Previously, ninja was using a Windows API call that only counted the number of processors in the first processor group, which resulted in only partial utilization of high-processing-power systems when no -j value was specified.

When building Chrome on a 72-core Windows system, this change has the following effect:

Before:
ninja detects 36 cores (i.e. one processor group only)
Average time to build Chrome over 10 builds: 5628 s

After:
ninja detects all 72 cores
Average time to build Chrome over 10 builds: 3215 s (57% of the time needed before)

This fixes #1603 .

@atetubou
Copy link
Contributor

You need to have -D_WIN32_WINNT=0x0601 in cygwin build?

@jessemckenna
Copy link
Contributor Author

Thanks so much @atetubou for the helpful suggestion! : )

@jhasse jhasse added the windows label Jul 24, 2019
@jonesmz

This comment was marked as abuse.

@jhasse
Copy link
Collaborator

jhasse commented Aug 4, 2019

What does that change mean in terms of the windows platforms that Ninja will execute on?

It means that Ninja requires Windows 7 (Windows Server 2008) now.

Does ninja target a specific windows platform? E.g. is Ninja still supporting windows XP? Vista? 8? 8.1?

At the moment: All of them. After this PR: Only 8 and 8.1.

For this PR to be merged, someone with a >64 core machine needs to test it. Also anyone with an unusual Windows setup (e.g. 2 CPUs) could give it a try.

@jonesmz

This comment was marked as abuse.

@jessemckenna
Copy link
Contributor Author

If losing support for older versions of Windows is not an option, the alternative would be for Windows users with > 64 processors to explicitly pass a -j value that reflects their actual number of processors. This could perhaps be reflected in the documentation somehow.

Re: testing, I believe there is a way to manually set the system group size (see: https://docs.microsoft.com/en-us/windows-hardware/drivers/devtest/boot-parameters-to-test-drivers-for-multiple-processor-group-support#create-multiple-processor-groups-by-changing-the-group-size). This change could be tested by setting group size to (number of processors / 2) to create two processor groups. I believe the ninja will then detect only half the system processors, i.e. one processor group.

@jonesmz

This comment was marked as abuse.

@atetubou
Copy link
Contributor

atetubou commented Aug 7, 2019

Maybe better to keep fallback to GetNativeSystemInfo so that ninja can run on older os for a while?

@jhasse
Copy link
Collaborator

jhasse commented Aug 7, 2019

Windows 7 reaches total end-of-life in January 2020.

Windows 7 would still be supported.

Maybe better to keep fallback to GetNativeSystemInfo so that ninja can run on older os for a while?

Do you still need XP/Vista support at Google? If not, I don't see who would honestly.

@jonesmz

This comment was marked as abuse.

@jhasse
Copy link
Collaborator

jhasse commented Aug 7, 2019

which would be end-of-life by the time it was included in an official release

That's just speculation.

@jonesmz

This comment was marked as abuse.

@atetubou
Copy link
Contributor

atetubou commented Aug 8, 2019

Maybe better to keep fallback to GetNativeSystemInfo so that ninja can run on older os for a while?

Do you still need XP/Vista support at Google? If not, I don't see who would honestly.

No, we don't use XP/Vista anymore. So I'm fine with this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ninja not making use of more than 64 CPUs
4 participants