JDK-8301661: Enhance os::pd_print_cpu_info on macOS and Windows #12403

MBaesken · 2023-02-03T08:25:09Z

Enhance os::pd_print_cpu_info on macOS and Windows by information about CPU frequency and caches.
On Windows , this info can be obtained by the Processor Power Information API or "powerbase" (CallNtPowerInformation , see https://learn.microsoft.com/en-us/windows/win32/api/powerbase/nf-powerbase-callntpowerinformation ); this is available since Windows Server 2003/XP.
On macOS, sysctlbyname can be used.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8301661: Enhance os::pd_print_cpu_info on macOS and Windows

Reviewers

Magnus Ihse Bursie (@magicus - Reviewer) ⚠️ Review applies to 9a311018
Lutz Schmidt (@RealLucy - Reviewer) ⚠️ Review applies to 9a311018
David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12403/head:pull/12403
$ git checkout pull/12403

Update a local copy of the PR:
$ git checkout pull/12403
$ git pull https://git.openjdk.org/jdk pull/12403/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12403

View PR using the GUI difftool:
$ git pr show -t 12403

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12403.diff

bridgekeeper · 2023-02-03T08:27:00Z

👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-02-03T08:28:35Z

@MBaesken The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-02-03T08:32:38Z

Webrevs

dholmes-ora · 2023-02-03T08:42:47Z

Okay the usual question when we augment the information that will appear in hs_err file: how safe is this going to be to call from a signal handler?

MBaesken · 2023-02-03T11:49:44Z

Okay the usual question when we augment the information that will appear in hs_err file: how safe is this going to be to call from a signal handler?

Hi David , I think the sysctl / sysctlbyname calls on macOS are rather safe (and we use this already for some time for other entries e.g. in os::print_summary_info in vm error reporting, so nothing really new. Not so sure about the Windows powerprof / powerbase API, this was not used before to my knowledge in OpenJDK. However we use both macOS and Windows APIs for some time in our internal JVM, no issues seen so far.

dholmes-ora · 2023-02-04T04:38:57Z

src/hotspot/os/windows/os_windows.cpp

+    ULONG MhzLimit;   // max specified processor clock frequency mult. by current processor thermal throttle limit
+    ULONG MaxIdleState;
+    ULONG CurrentIdleState;
+} PROCESSOR_POWER_INFORMATION, *PTR_PROCESSOR_POWER_INFORMATION;


We don't need the *PTR_PROCESSOR_POWER_INFORMATION

dholmes-ora · 2023-02-04T04:40:43Z

src/hotspot/os/windows/os_windows.cpp

 #include <shlobj.h>

 #include <malloc.h>
+#include <powrprof.h>


The header should be powerbase.h

dholmes-ora · 2023-02-04T04:44:08Z

src/hotspot/os/windows/os_windows.cpp

+// additional lib needed for PowerProf functionality
+#pragma comment(lib, "Powrprof.lib")


Not sure about this mechanism versus adding it to Makefile ??

dholmes-ora · 2023-02-04T04:50:22Z

src/hotspot/os/windows/os_windows.cpp

+  SYSTEM_INFO si;
+  GetSystemInfo(&si);
+
+  PROCESSOR_POWER_INFORMATION ppi;
+  size_t sz_check = sizeof(ppi) * (size_t)si.dwNumberOfProcessors;


We already have dwNumberOfProcessors available in processor_count() - though we may have to zero-initialize it and check for zero in case we crash before we've done os::init

That said from what I'm reading if systems have more than 64 processors they will be divided into processor groups and the max number will be 64. However IIUC CallNtPowerInformation will expect a buffer big enough for the true number of logical processors >64.

MBaesken · 2023-02-06T13:12:16Z

Hi David, thanks for the comments. I did a few adjustments following your comments. Regarding

Not sure about this mechanism versus adding it to Makefile ??

Both works. From what I see, currently we use the Makefile to set the lib deps, should I remove the pragma and use the Makefile ?
Regarding the processor count , probably we do not want to malloc anyway in pd_print_cpu_info so we have to live with the space buf provides.
Btw what you think about shortening the info, e.g. check if all processors have the same (M)Hz values and if it is the case just print once, this might look better and much less redundant in the output ?

dholmes-ora · 2023-02-07T04:30:09Z

From what I see, currently we use the Makefile to set the lib deps, should I remove the pragma and use the Makefile ?

I'm asking the build folk if they have any preference/concerns.

Regarding the processor count , probably we do not want to malloc anyway in pd_print_cpu_info so we have to live with the space buf provides.

True. I still think we might reuse processor_count(). Showing zero very early during VM init is not really an issue.

Btw what you think about shortening the info, e.g. check if all processors have the same (M)Hz values and if it is the case just print once, this might look better and much less redundant in the output ?

As this is intended as fairly general information shortening would be good - especially if the values reported are the same. Do we need to report all processors, or can we just use one as representative?

MBaesken · 2023-02-08T14:58:56Z

Hi David , I did another small adjustment. Reused os::processor_count if it delivers a value larger than 0. And adjusted the output a bit.

especially if the values reported are the same

On my notebook I noticed that the current idle state sometimes differs among processors. So it is not always the same.
Maybe it makes sense to print the current idle state separately for all processors and if possible, output the other values just once .

ProcessorInformation for all processors: Max Mhz: 2611, Current Mhz: 2611, Mhz Limit: 2611 MaxIdleState: 2 CurrentIdleState for processors 0 - 15: 2 2 2 2 2 2 2 2 2 1 2 2 2 2 2 2

dholmes-ora · 2023-02-08T20:45:10Z

With regards to the pragma I had the following response from @magicus :

Oh, please don't do that. We could perhaps have gone down that route, but we didn't. If you do that, there won't be a single source of truth on where to look for library linkage. We had a bug a year or two ago that ended up in us loading a library which nobody expected or realized due to a windows pragma. So, basically, there's nothing inherently wrong with this (just a bit non-standard from an old-school unix point of view), but it does not fit with the current model of the JDK

dholmes-ora · 2023-02-08T20:46:57Z

On my notebook I noticed that the current idle state sometimes differs among processors. So it is not always the same.
Maybe it makes sense to print the current idle state separately for all processors and if possible, output the other values just once .

I have no idea what possible use the "idle state" information would be in any case. We don't have that per-processor info on non-Windows.

magicus · 2023-02-09T19:18:33Z

/label build

magicus

Build changes look good now. Thanks for removing the pragma.

/reviewers 2

openjdk · 2023-02-09T19:19:40Z

@magicus
The build label was successfully added.

openjdk · 2023-02-09T19:20:31Z

@MBaesken This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8301661: Enhance os::pd_print_cpu_info on macOS and Windows

Reviewed-by: ihse, lucy, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 274 new commits pushed to the master branch:

bb3dfd6: 8302879: doc/building.md update link to jtreg builds
36a0822: 8302369: Reduce the stack size of the C1 compiler
0bf3a53: 8302599: Extend ASan support to Microsoft Visual C++
c7517b3: 8302525: Write a test to check various components send Events while mouse and key are used simultaneously
9a79722: 8299234: JMX Repository.query performance
e47e9ec: 8300658: memory_and_swap_limit() reporting wrong values on systems with swapaccount=0
7cf7e0a: 8302070: Factor null-check into load_klass() calls
e731695: 8302882: Newly added test javax/swing/JFileChooser/JFileChooserFontReset.java fails with HeadlessException
b5a7426: 8301749: Tracking malloc pooled memory size
6ac5e05: 8302068: Serial: Refactor oop closures used in Young GC
... and 264 more: https://git.openjdk.org/jdk/compare/af474ce35997315774e408f2e8a1beecf8349c75...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2023-02-09T19:21:07Z

@magicus
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

MBaesken · 2023-02-10T08:17:32Z

I have no idea what possible use the "idle state" information would be in any case. We don't have that per-processor info on non-Windows.

Hi David, there is some info about the idle states at https://learn.microsoft.com/en-us/windows-hardware/test/wpt/cpu-analysis ; if you think that currently, with the info only available on Windows, it makes not that much sense to add, it I can omit it for now. Would probably make it easier to scan through the cpu-infos and observe same MHz numbers for all and then print the same info for all processors (as I said, where I tested the MHz values were the same for all CPUs only the current idle states varied).

MBaesken · 2023-02-10T08:18:30Z

Hi Magnus, thanks for the review !

RealLucy

Changes look good to me

MBaesken · 2023-02-15T17:53:02Z

Hi David are you okay with the latest version ?

dholmes-ora · 2023-02-16T02:12:10Z

Hi David are you okay with the latest version ?

I thought you were looking at reducing the output:

Maybe it makes sense to print the current idle state separately for all processors and if possible, output the other values just once .

I don't think idle states are particularly useful, so I'd prefer to see similar output to non-Windows rather than per-processor info that is the same for everything of interest.

…ll proces only once

dholmes-ora

General approach seems okay. A couple of queries and nits.

Thanks.

src/hotspot/os/windows/os_windows.cpp

dholmes-ora · 2023-02-17T05:17:59Z

src/hotspot/os/windows/os_windows.cpp

+        if (MaxMhz != (int) pppi->MaxMhz ||
+            CurrentMhz != (int) pppi->CurrentMhz ||
+            MhzLimit != (int) pppi->MhzLimit) {
+          same_vals_for_all_cpus = false;


You could break once this is false.

Added a break.

dholmes-ora · 2023-02-17T05:20:34Z

src/hotspot/os/windows/os_windows.cpp

+  }
+
+  size_t sz_check = sizeof(PROCESSOR_POWER_INFORMATION) * (size_t)proc_count;
+  NTSTATUS status = ::CallNtPowerInformation(ProcessorInformation, NULL, 0, buf, (ULONG) buflen);


How is the caller supposed to know what size buffer to pass? Are they just supposed to guess and hope it is big enough??

src/hotspot/os/windows/os_windows.cpp

dholmes-ora · 2023-02-17T05:25:15Z

src/hotspot/os/windows/os_windows.cpp

+        }
+      }
+      // avoid iteration in case buf is too small to hold all proc infos
+      if (sz_check > buflen) break;


In this case shouldn't the function have returned STATUS_BUFFER_TOO_SMALL?

src/hotspot/os/windows/os_windows.cpp

MBaesken · 2023-02-17T08:24:05Z

Hi David I adjusted the output, renamed some variables and added a break where you suggested.

In this case shouldn't the function have returned STATUS_BUFFER_TOO_SMALL

I think this would be the case, but I was not sure so I added the check.

How is the caller supposed to know what size buffer to pass? Are they just supposed to guess and hope it is big enough??

I think this is the same for most of the print_ functions from
src/hotspot/share/runtime/os.hpp
getting a buffer 'buf' and a buf-length value, they are unfortunately not very specific on the size passed.

https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/os.hpp#L754

// Print out system information; they are called by fatal error handler.
// Output format may be different on different platforms.

dholmes-ora

I'm still a little dubious about the buffer size check but lets see how this goes.

Thanks.

MBaesken · 2023-02-21T08:15:10Z

Hi David and Lutz, thanks for the reviews.

/integrate

openjdk · 2023-02-21T08:18:07Z

Going to push as commit 9145670.
Since your change was applied there have been 280 commits pushed to the master branch:

aa10f0d: 8302151: BMPImageReader throws an exception reading BMP images
16a4f02: 8302150: Speed up compiler/codegen/Test7100757.java
17274c7: 8302146: Move TestOverloadCompileQueues.java to tier3
29f392e: 8299522: Incorrect size of Approve button in custom JFileChooser
43c71dd: 8302453: RISC-V: Add support for small width vector operations
91a2b5e: 8302905: arm32 Raspberry Pi OS build broken by JDK-8301494
bb3dfd6: 8302879: doc/building.md update link to jtreg builds
36a0822: 8302369: Reduce the stack size of the C1 compiler
0bf3a53: 8302599: Extend ASan support to Microsoft Visual C++
c7517b3: 8302525: Write a test to check various components send Events while mouse and key are used simultaneously
... and 270 more: https://git.openjdk.org/jdk/compare/af474ce35997315774e408f2e8a1beecf8349c75...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-21T08:18:22Z

@MBaesken Pushed as commit 9145670.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8301661

c73a490

openjdk bot added the rfr Pull request is ready for review label Feb 3, 2023

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Feb 3, 2023

dholmes-ora reviewed Feb 4, 2023

View reviewed changes

Some adjustments mostly suggested by David

08bba6d

Reuse processor count if possible, adjust output a bit

12cf8db

link powrprof lib without pragma

9a31101

magicus approved these changes Feb 9, 2023

View reviewed changes

openjdk bot added the build build-dev@openjdk.org label Feb 9, 2023

openjdk bot added the ready Pull request is ready to be integrated label Feb 9, 2023

openjdk bot removed the ready Pull request is ready to be integrated label Feb 9, 2023

RealLucy approved these changes Feb 14, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 14, 2023

Do not print idle state infos on Windows, print same MHz values for a…

2811225

…ll proces only once

dholmes-ora reviewed Feb 17, 2023

View reviewed changes

Rename variables, adjust output, add a break

439644e

dholmes-ora approved these changes Feb 21, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 21, 2023

openjdk bot closed this Feb 21, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 21, 2023

		// additional lib needed for PowerProf functionality
		#pragma comment(lib, "Powrprof.lib")

JDK-8301661: Enhance os::pd_print_cpu_info on macOS and Windows #12403

JDK-8301661: Enhance os::pd_print_cpu_info on macOS and Windows #12403

Uh oh!

Conversation

MBaesken commented Feb 3, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 3, 2023

Uh oh!

openjdk bot commented Feb 3, 2023

Uh oh!

mlbridge bot commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora commented Feb 3, 2023

Uh oh!

MBaesken commented Feb 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MBaesken commented Feb 6, 2023

Uh oh!

dholmes-ora commented Feb 7, 2023

Uh oh!

MBaesken commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dholmes-ora commented Feb 8, 2023

Uh oh!

dholmes-ora commented Feb 8, 2023

Uh oh!

magicus commented Feb 9, 2023

Uh oh!

magicus left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Feb 9, 2023

Uh oh!

openjdk bot commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Feb 9, 2023

Uh oh!

MBaesken commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MBaesken commented Feb 10, 2023

Uh oh!

RealLucy left a comment

Choose a reason for hiding this comment

Uh oh!

MBaesken commented Feb 15, 2023

Uh oh!

dholmes-ora commented Feb 16, 2023

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

MBaesken commented Feb 3, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 3, 2023 •

edited

Loading

MBaesken commented Feb 8, 2023 •

edited

Loading

openjdk bot commented Feb 9, 2023 •

edited

Loading

MBaesken commented Feb 10, 2023 •

edited

Loading