RFR: 6942632: Hotspot should be able to use more than 64 logical processors on Windows [v2]

[Johan Sjölen]

On Thu, 15 Feb 2024 03:57:22 GMT, Saint Wesonga <duke at openjdk.org> wrote:

>> Windows processor groups support a maximum of 64 logical processors per processor group. OpenJDK currently uses the GetSystemInfo API to get the number of processors on the system. GetSystemInfo only returns the number of processors in the current processor group, which is at most 64. Therefore, OpenJDK needs to be updated to support the use of all available processors across all Windows processor groups. However, this should only be done when running on Windows 11 or Windows Server 2022 or later Windows operating systems.
>> 
>> Starting with Windows 11 and Windows Server 2022, applications are no longer constrained by default to a single processor group. Instead, processes and their threads have processor affinities that by default span all processors in the system, across multiple groups on machines with more than 64 processors. This is in contrast to previous operating system versions where the processor group affinity APIs needed to be used to assign each thread a specific processor group. See  https://learn.microsoft.com/en-us/windows/win32/procthread/processor-groups#behavior-starting-with-windows-11-and-windows-server-2022
>> 
>> The key changes required are:
>> 
>> 1. Detecting Windows 11 or Windows Server 2022 or later.
>> 1. Using the The GetLogicalProcessorInformationEx function to correctly retrieve the total number of processors available on the system. This function was introduced in Windows 7 so its availability needs to be explicitly determined.
>> 1. Updating the calculation of the number of active processors.
>>     1. Processor affinities can be set in a Windows job object and if the JVM is launched in a job object, these affinities restrict the processors that the JVM can use (regardless of how many processors the system has).
>>     1. The JVM can also be launched from the command prompt  by using the start command to set processor affinities instead of using a job object. This command only sets the processor affinity of the primary processor group because it is a 64-bit affinity mask. On systems with over 64 processors, passing an all 1s 64-bit affinity mask to the start command is treated as equivalent to no processor affinity restrictions for the JVM. Any other affinity mask passed to the start command explicitly sets the number of available processors for the JVM and is necessarily at most 63, even if the machine has more than 64 processors.
>>     1. If no affinity restrictions are present, then all cores on the system will be available t...
>
> Saint Wesonga has updated the pull request incrementally with 10 additional commits since the last revision:
> 
>  - Specify required libraries for GTestWrapper
>  - Access arrays using incremented base pointer
>  - Avoid copying structs in loops
>  - Restore deleted comment
>  - Add warning for unexpected group counts
>  - Add warning when processors is 0
>  - Remove unused local
>  - Add product flag for enabling all processor groups
>  - Remove check for existence of GetLogicalProcessorInformationEx
>    
>    Windows versions preceding Windows 7 are not supported
>  - Fix memory leak

Hi,

Please avoid implicit casts from int to bool. I noted two examples, but there are more in the code. I have a preference for avoiding unnecessary indentation, and I noted one place where this can be done, but consider that a nit and not required :-).

Other than that, the code seems correct and the idea itself sound.

src/hotspot/os/windows/os_windows.cpp line 4072:

> 4070:   DWORD job_object_information_length = 0;
> 4071: 
> 4072:   if (!QueryInformationJobObject(nullptr, JobObjectGroupInformationEx, nullptr, 0, &job_object_information_length)) {

This is an implicit cast from int to bool, and there is no else-branch. Instead:

```c++
if (QueryInformationJobObject(nullptr, JobObjectGroupInformationEx, nullptr, 0, &job_object_information_length) == 0) {
  return processors;
}
// the rest of the code


This reduces indentation on a large body of code and avoids the implicit cast. Please consider the indentation change as a nit and the removal of the implicit cast as required.

src/hotspot/os/windows/os_windows.cpp line 4079:

> 4077:       job_object_information = os::malloc(job_object_information_length, mtInternal);
> 4078:       if (job_object_information != nullptr) {
> 4079:           if (QueryInformationJobObject(nullptr, JobObjectGroupInformationEx, job_object_information, job_object_information_length, &job_object_information_length)) {

Implicit cast.

src/hotspot/os/windows/os_windows.cpp line 4082:

> 4080:             DWORD groups_found = job_object_information_length / sizeof(GROUP_AFFINITY);
> 4081:             if (groups_found != group_count) {
> 4082:               warning("Unexpected processor group count->%ld. Expected %ld processor groups.", groups_found, group_count);

Why `->`? Perhaps ": " or " of " is better

-------------

PR Review: https://git.openjdk.org/jdk/pull/17576#pullrequestreview-1882399698
PR Review Comment: https://git.openjdk.org/jdk/pull/17576#discussion_r1490787367
PR Review Comment: https://git.openjdk.org/jdk/pull/17576#discussion_r1490791767
PR Review Comment: https://git.openjdk.org/jdk/pull/17576#discussion_r1490794912