8326446: The User and System of jdk.CPULoad on Apple M1 are inaccurate #17976

yanglong1010 · 2024-02-23T07:49:45Z

hi

I would like to fix this.

As the description in JDK-8326446.
JFR uses task_info() with flavor TASK_ABSOLUTETIME_INFO to read User and System time. it is not reliable on Apple m1.

Libc provides the times function, which uses TASK_BASIC_INFO_COUNT and TASK_THREAD_TIMES_INFO_COUNT. It will also return the real time of the process, and the time unit is the same, which is suitable for solving this problem.

I ran test/jdk/jdk/jfr/event/os/TestCPULoad.java and passed.

I would appreciate it if you could review this.

Progress

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

Issue

JDK-8326446: The User and System of jdk.CPULoad on Apple M1 are inaccurate (Bug - P3)

Reviewers

Markus Grönlund (@mgronlun - Reviewer)
Erik Gahlin (@egahlin - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17976

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-02-23T07:50:27Z

👋 Welcome back yanglong1010! 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 · 2024-02-23T07:51:13Z

@yanglong1010 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 · 2024-02-23T07:54:14Z

Webrevs

01: Full - Incremental (4cf1dba3)
00: Full (20a3e526)

yanglong1010 · 2024-02-23T08:00:14Z

/label add hotspot-jfr

openjdk · 2024-02-23T08:01:18Z

@yanglong1010
The hotspot-jfr label was successfully added.

mgronlun · 2024-02-28T11:46:35Z

Seems reasonable.

Is there a risk of introducing a regression for existing platforms? Did the result on x64 turn out equivalent to pre-change?

mgronlun · 2024-02-28T12:03:02Z

The units are changed from nanos to ticks, but this is opaque because doubles are used to represent only the ratios?

yanglong1010 · 2024-02-28T14:37:44Z

The units are changed from nanos to ticks, but this is opaque because doubles are used to represent only the ratios?

Yes

yanglong1010 · 2024-02-28T14:42:26Z

Seems reasonable.

Is there a risk of introducing a regression for existing platforms? Did the result on x64 turn out equivalent to pre-change?

I ran the pre- and post-change versions on MacOS X64, and I can see that the results are consistent.

The test will occupy half of the CPUs, so the sum of User and System is close to 50%. There are other processes on the host that occupy the CPUs, so the utilization rate of MachineTotal is higher than 50%.

OS Information

Time     OS Version
-------- --------------------------------------------------------------------------------------------------------------
22:28:13 uname: Darwin 21.6.0 Darwin Kernel Version 21.6.0: Mon Apr 24 21:10:53 PDT 2023; root:xnu-8020.240.18.701.5...

CPU Information

Time     Type                                    Description                             Sockets Cores Hardware Threads
-------- --------------------------------------- --------------------------------------- ------- ----- ----------------
22:28:13 Intel Haswell (HT) SSE SSE2 SSE3 SSS... Brand: Intel(R) Core(TM) i7-4770HQ C...       1     4                8

                                     CPU Load (pre fix, I ran the changed version first, so the time is newer)

Time                         JVM User           JVM System           Machine Total
------------------ ------------------ -------------------- -----------------------
22:28:15                       10.41%               39.19%                  76.21%
22:28:16                       10.31%               39.46%                  70.88%
22:28:17                       10.30%               39.46%                  68.58%
22:28:18                       10.30%               39.47%                  67.96%

                                     CPU Load (post fix)

Time                         JVM User           JVM System           Machine Total
------------------ ------------------ -------------------- -----------------------
22:25:58                       10.40%               38.99%                  82.34%
22:25:59                       10.37%               39.50%                  82.15%
22:26:00                       10.27%               39.23%                  68.65%
22:26:01                       10.37%               39.63%                  68.25%

mgronlun · 2024-02-29T11:21:56Z

Thank you for confirming.

openjdk · 2024-02-29T11:23:11Z

⚠️ @yanglong1010 the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout fix-8326446
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

openjdk · 2024-02-29T11:23:12Z

@yanglong1010 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:

8326446: The User and System of jdk.CPULoad on Apple M1 are inaccurate

Reviewed-by: mgronlun, egahlin

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 160 new commits pushed to the master branch:

784f11c: 8327238: Remove MetadataAllocationFailALot* develop flags
d7273ac: 8320646: RISC-V: C2 VectorCastHF2F
53c4714: 8327501: Common ForkJoinPool prevents class unloading in some cases
1261740: 8327283: RISC-V: Minimal build failed after JDK-8319716
f54e598: 8327172: C2 SuperWord: data node in loop has no input in loop: replace assert with bailout
4018341: 8327379: Make TimeLinearScan a develop flag
1bd4abf: 8327434: Test java/util/PluggableLocale/TimeZoneNameProviderTest.java timed out
ddcd6de: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
3d37b28: 8327173: HotSpot Style Guide needs update regarding nullptr vs NULL
989fc3e: 8325878: Require minimum Clang version 13
... and 150 more: https://git.openjdk.org/jdk/compare/c4409eafc418c1e7a4ca2a2a522b6855c70c0f8c...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mgronlun, @egahlin) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

yanglong1010 · 2024-02-29T11:36:15Z

/integrate

openjdk · 2024-02-29T11:37:01Z

@yanglong1010
Your change (at version 4cf1dba) is now ready to be sponsored by a Committer.

yanglong1010 · 2024-02-29T11:43:10Z

@mgronlun Thanks for your review. Does this patch need a second review ?

mgronlun · 2024-02-29T12:52:31Z

I think it needs a second review. Also, I am not very versed in Mac internals.

yanglong1010 · 2024-02-29T13:21:56Z

I think it needs a second review. Also, I am not very versed in Mac internals.

Thanks. Would you help me to invite the second reviewer?

yanglong1010 · 2024-03-04T07:09:06Z

@egahlin Could I have a second review of this patch.

egahlin · 2024-03-07T00:09:41Z

@egahlin Could I have a second review of this patch.

Looks reasonable, but why was this removed:

if(!now_in_nanos(&total_cpu_nanos)) { return OS_ERR; }

yanglong1010 · 2024-03-07T01:57:24Z

Because the times libc api returns the real time of the process. So we don't need to calculate the total_cpu_nanos, just use the return value of times.

struct tms buf;
clock_t jvm_real = times(&buf);
if (jvm_real == (clock_t) (-1)) {
  return OS_ERR;
}

...

uint64_t delta = active_processor_count * (jvm_real - _jvm_real);

egahlin · 2024-03-07T14:34:41Z

Because the times libc api returns the real time of the process.

OK, I had a vague memory about some other issue, but I don't remember and it probably doesn't apply.

D-D-H · 2024-03-08T01:58:45Z

/sponsor

openjdk · 2024-03-08T01:59:32Z

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

7c5e6e7: 8327147: Improve performance of Math ceil, floor, and rint for x86
972e81d: 8326611: Clean up vmTestbase/nsk/stress/stack tests
5aae803: 8327390: JitTester: Implement temporary folder functionality
784f11c: 8327238: Remove MetadataAllocationFailALot* develop flags
d7273ac: 8320646: RISC-V: C2 VectorCastHF2F
53c4714: 8327501: Common ForkJoinPool prevents class unloading in some cases
1261740: 8327283: RISC-V: Minimal build failed after JDK-8319716
f54e598: 8327172: C2 SuperWord: data node in loop has no input in loop: replace assert with bailout
4018341: 8327379: Make TimeLinearScan a develop flag
1bd4abf: 8327434: Test java/util/PluggableLocale/TimeZoneNameProviderTest.java timed out
... and 153 more: https://git.openjdk.org/jdk/compare/c4409eafc418c1e7a4ca2a2a522b6855c70c0f8c...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-08T01:59:40Z

@D-D-H @yanglong1010 Pushed as commit 8dbd4b3.

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

yanglong1010 · 2024-03-08T02:00:30Z

Thank you all.

yanglong1010 · 2024-03-08T02:50:26Z

/backport jdk21u-dev

openjdk · 2024-03-08T02:50:38Z

@yanglong1010 To use the /backport command, you need to be in the OpenJDK census and your GitHub account needs to be linked with your OpenJDK username (how to associate your GitHub account with your OpenJDK username).

8326446: The User and System of jdk.CPULoad on Apple M1 are inaccurate

20a3e52

openjdk bot added the rfr Pull request is ready for review label Feb 23, 2024

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Feb 23, 2024

openjdk bot added the hotspot-jfr hotspot-jfr-dev@openjdk.org label Feb 23, 2024

update copyright header

4cf1dba

mgronlun approved these changes Feb 29, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 29, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 29, 2024

egahlin approved these changes Mar 7, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 8, 2024

openjdk bot closed this Mar 8, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8326446: The User and System of jdk.CPULoad on Apple M1 are inaccurate #17976

8326446: The User and System of jdk.CPULoad on Apple M1 are inaccurate #17976

yanglong1010 commented Feb 23, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 23, 2024

openjdk bot commented Feb 23, 2024

mlbridge bot commented Feb 23, 2024 •

edited

Loading

yanglong1010 commented Feb 23, 2024

openjdk bot commented Feb 23, 2024

mgronlun commented Feb 28, 2024

mgronlun commented Feb 28, 2024

yanglong1010 commented Feb 28, 2024

yanglong1010 commented Feb 28, 2024 •

edited

Loading

mgronlun commented Feb 29, 2024

openjdk bot commented Feb 29, 2024

openjdk bot commented Feb 29, 2024 •

edited

Loading

yanglong1010 commented Feb 29, 2024

openjdk bot commented Feb 29, 2024

yanglong1010 commented Feb 29, 2024

mgronlun commented Feb 29, 2024

yanglong1010 commented Feb 29, 2024

yanglong1010 commented Mar 4, 2024 •

edited

Loading

egahlin commented Mar 7, 2024

yanglong1010 commented Mar 7, 2024

egahlin commented Mar 7, 2024

D-D-H commented Mar 8, 2024

openjdk bot commented Mar 8, 2024

openjdk bot commented Mar 8, 2024

yanglong1010 commented Mar 8, 2024

yanglong1010 commented Mar 8, 2024

openjdk bot commented Mar 8, 2024

8326446: The User and System of jdk.CPULoad on Apple M1 are inaccurate #17976

8326446: The User and System of jdk.CPULoad on Apple M1 are inaccurate #17976

Conversation

yanglong1010 commented Feb 23, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Feb 23, 2024

openjdk bot commented Feb 23, 2024

mlbridge bot commented Feb 23, 2024 • edited Loading

Webrevs

yanglong1010 commented Feb 23, 2024

openjdk bot commented Feb 23, 2024

mgronlun commented Feb 28, 2024

mgronlun commented Feb 28, 2024

yanglong1010 commented Feb 28, 2024

yanglong1010 commented Feb 28, 2024 • edited Loading

mgronlun commented Feb 29, 2024

openjdk bot commented Feb 29, 2024

openjdk bot commented Feb 29, 2024 • edited Loading

yanglong1010 commented Feb 29, 2024

openjdk bot commented Feb 29, 2024

yanglong1010 commented Feb 29, 2024

mgronlun commented Feb 29, 2024

yanglong1010 commented Feb 29, 2024

yanglong1010 commented Mar 4, 2024 • edited Loading

egahlin commented Mar 7, 2024

yanglong1010 commented Mar 7, 2024

egahlin commented Mar 7, 2024

D-D-H commented Mar 8, 2024

openjdk bot commented Mar 8, 2024

openjdk bot commented Mar 8, 2024

yanglong1010 commented Mar 8, 2024

yanglong1010 commented Mar 8, 2024

openjdk bot commented Mar 8, 2024

yanglong1010 commented Feb 23, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 23, 2024 •

edited

Loading

yanglong1010 commented Feb 28, 2024 •

edited

Loading

openjdk bot commented Feb 29, 2024 •

edited

Loading

yanglong1010 commented Mar 4, 2024 •

edited

Loading