8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM by phohensee · Pull Request #13814 · openjdk/jdk

phohensee · 2023-05-04T19:54:57Z

Please review this addition to com.sun.management.ThreadMXBean that returns the total number of bytes allocated on the Java heap since JVM launch by both terminated and live threads.

Because this PR adds a new interface method, I've updated the JMM_VERSION to 4, but would be happy to update it to 3_1 instead.

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
Change requires CSR request JDK-8307396 to be approved

Issues

JDK-8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM
JDK-8307396: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM (CSR)

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Mandy Chung (@mlchung - Reviewer) ⚠️ Review applies to 5f2f86bb

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13814

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-04T19:55:40Z

👋 Welcome back phh! 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-05-04T19:57:39Z

@phohensee The following labels will be automatically applied to this pull request:

hotspot-runtime
jmx
serviceability

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

mlbridge · 2023-05-04T20:04:14Z

Webrevs

src/hotspot/share/services/management.cpp

shipilev

Some comments follow.

src/hotspot/share/services/threadService.hpp

src/java.management/share/classes/sun/management/ThreadImpl.java

src/jdk.management/share/classes/com/sun/management/ThreadMXBean.java

test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java

test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemoryArray.java

test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java

src/hotspot/share/services/management.cpp

src/hotspot/share/include/jmm.h

src/hotspot/share/services/management.cpp

simonis

Looks good in general. Please find my comments inline.

src/hotspot/share/include/jmm.h

src/hotspot/share/services/management.cpp

…ava heap by the JVM

src/hotspot/share/services/management.cpp

src/hotspot/share/services/threadService.cpp

src/jdk.management/share/classes/com/sun/management/ThreadMXBean.java

src/hotspot/share/runtime/atomic.hpp

src/jdk.management/share/classes/com/sun/management/ThreadMXBean.java

src/hotspot/share/services/management.cpp

phohensee · 2023-05-24T16:36:05Z

Yes, it seems to have happened when I did a merge of this branch with jdk master, which latter must have been in a strange state. Unfortunately, git merge didn't complain at all, so I didn't check before pushing.

dholmes-ora · 2023-05-25T07:08:00Z

@phohensee not clear if you saw my comment a couple of days ago about why the monotonic increasing check in the test can't work.

phohensee · 2023-05-25T14:11:51Z

Yes, I saw your comment and have been thinking about how to fix it, but haven't come up with anything. Ramki (ysr) pointed out that walking the thread list using SMR is very expensive when there are 1000s of threads, and that there isn't much overhead to collecting per-thread statistics: a thread local counter is incremented when TLABs are retired or there are direct allocations of large objects in eden or the old gen. So, he suggested adding an atomic increment of a global counter whenever a thread local counter is incremented. Atomic operations have been completing in the LLC (i.e., on-chip) for some time now, so the overhead is pretty small. What do you think, shall I reimplement that way?

dholmes-ora · 2023-05-26T00:06:17Z

A global allocation counter sounds like a potential bottleneck to me and everyone has to pay for it. I'd want to see broad performance measurements for that and hear what the GC folk have to say.

dholmes-ora · 2023-05-26T00:07:12Z

A global counter would also need to resurrect Atomic::add(jlong) for all platforms.

dholmes-ora · 2023-05-26T02:30:34Z

The monotonicity issue can be addressed by tracking the last total and never reporting a smaller value. (Much as we used to cache the current time to avoid it going backwards.)

phohensee · 2023-05-26T16:37:07Z

I added a high water mark to getTotalThreadAllocatedBytes (under a lock, lest we have a monotonicity issue on the high water mark), and updated the tests per Mandy's request.

mlchung · 2023-05-26T17:24:19Z

Thanks for updating the test. The spec looks good. I'll leave the hotspot implementation for David and others to approve.

phohensee · 2023-05-26T18:56:26Z

Thanks for your review, Mandy.

dholmes-ora

High watermark changes seem fine in principle but not sure about borrowing the MonitoringSupport_lock to do the locking. At a minimum this comment should be expanded:

./share/runtime/mutexLocker.hpp:extern Mutex*   MonitoringSupport_lock;          // Protects updates to the serviceability memory pools.

dholmes-ora · 2023-05-29T02:02:21Z

test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java

        return currSize;
    }

+    private static void reportUnexpected(Exception e, String reason) {


nit: 'when' or 'where' seems a more appropriate name than 'reason'

Changed to "when".

…to MonitoringSupport_lock declaration

phohensee · 2023-05-29T17:45:42Z

Changed the MonitoringSupport_lock comment to "Protects updates to the serviceability memory pools and allocated memory high water mark". The lock was introduced to serialize updates to the G1 memory pool stats, but given that doesn't happen very often, and the name, imo it's reasonable to use it in a more general fashion.

dholmes-ora

Nothing further from me. Thanks.

phohensee · 2023-05-30T13:43:01Z

Thanks for your in-depth review!

phohensee · 2023-05-30T13:43:15Z

/integrate

openjdk · 2023-05-30T13:44:17Z

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

15e0285: 8309110: Build failure after JDK-8307795 due to warnings in micro-benchmark StoreMaskTrueCount.java
4526282: 8308977: gtest:codestrings fails on riscv
f600d03: 8307795: AArch64: Optimize VectorMask.truecount() on Neon
07f2070: 8309095: Remove UTF-8 character from TaskbarPositionTest.java
2b186e2: 8309042: MemorySegment::reinterpret cleanup action is not called for all overloads
78aac24: 8308881: Strong CLD oop handle roots are demoted to non-roots concurrently
1f1f604: 8302670: use-after-free related to PhaseIterGVN interaction with Unique_Node_List and Node_Stack
d35a550: 8309077: Problemlist compiler/jvmci/TestUncaughtErrorInCompileMethod.java
457e1cb: 8308817: RISC-V: Support VectorTest node for Vector API
7508d9f: 8308906: Make CIPrintCompilerName a diagnostic flag
... and 14 more: https://git.openjdk.org/jdk/compare/77c5adb09e89e013c4bc4982f541110bf76e83a7...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-30T13:44:30Z

@phohensee Pushed as commit 3eced01.

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

8304074: [JMX] Add an approximation of JVM process allocated bytes

d78ec8f

openjdk bot added csr Pull request needs approved CSR before integration rfr Pull request is ready for review labels May 4, 2023

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot-runtime hotspot-runtime-dev@openjdk.org jmx jmx-dev@openjdk.org labels May 4, 2023

dholmes-ora reviewed May 5, 2023

View reviewed changes

src/hotspot/share/services/management.cpp Outdated Show resolved Hide resolved

dholmes-ora reviewed May 5, 2023

View reviewed changes

src/hotspot/share/services/management.cpp Outdated Show resolved Hide resolved

phohensee changed the title ~~8304074: [JMX] Add an approximation of JVM process allocated bytes~~ 8304074: [JMX] Add an approximation of JVM bytes allocated on the Java heap May 5, 2023

phohensee changed the title ~~8304074: [JMX] Add an approximation of JVM bytes allocated on the Java heap~~ 8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM May 5, 2023

shipilev reviewed May 5, 2023

View reviewed changes

simonis reviewed May 5, 2023

View reviewed changes

src/hotspot/share/include/jmm.h Outdated Show resolved Hide resolved

src/hotspot/share/include/jmm.h Outdated Show resolved Hide resolved

src/hotspot/share/services/management.cpp Outdated Show resolved Hide resolved

src/hotspot/share/services/management.cpp Outdated Show resolved Hide resolved

phohensee added 4 commits May 5, 2023 20:41

8304074: [JMX] Add an approximation of total bytes allocated on the J…

460c00e

…ava heap by the JVM

8304074: [JMX] Add an approximation of total bytes allocated on the J…

1001b66

…ava heap by the JVM

8304074: [JMX] Add an approximation of total bytes allocated on the J…

2e2adc0

…ava heap by the JVM

8304074: [JMX] Add an approximation of total bytes allocated on the J…

7b92226

…ava heap by the JVM

dholmes-ora reviewed May 8, 2023

View reviewed changes

src/hotspot/share/services/management.cpp Outdated Show resolved Hide resolved

src/hotspot/share/services/threadService.cpp Outdated Show resolved Hide resolved

src/jdk.management/share/classes/com/sun/management/ThreadMXBean.java Outdated Show resolved Hide resolved

phohensee added 7 commits May 8, 2023 20:29

8304074: CSR comment updates

9322819

8304074: Implement 32-bit linux Atomic::add()

4161677

CSR comment updates

d9039f3

CSR comment updates

fcabf08

Implement 32-bit linux Atomic::add()

0bf27fe

Implement 32-bit linux Atomic::add()

c592c8a

Implement 32-bit linux Atomic::add()

75630a0

dholmes-ora reviewed May 9, 2023

View reviewed changes

src/hotspot/share/runtime/atomic.hpp Outdated Show resolved Hide resolved

dholmes-ora reviewed May 9, 2023

View reviewed changes

src/jdk.management/share/classes/com/sun/management/ThreadMXBean.java Outdated Show resolved Hide resolved

dholmes-ora reviewed May 9, 2023

View reviewed changes

src/hotspot/share/services/management.cpp Outdated Show resolved Hide resolved

phohensee added 2 commits May 24, 2023 16:41

Merge master

7b90091

Merge master

96cb0e5

phohensee added 4 commits May 26, 2023 15:32

8304074: Refactor test exception handling

c247333

Merge branch 'master' into 8304074

47375cc

8304074: Add getTotalAllocatedBytes high water mark

8c0d8a2

8304074: Remove extra file

5f2f86b

mlchung approved these changes May 26, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 26, 2023

dholmes-ora reviewed May 29, 2023

View reviewed changes

8304074: Change reportUnsupported argument name to when, add comment …

48a5852

…to MonitoringSupport_lock declaration

dholmes-ora approved these changes May 30, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 30, 2023

openjdk bot closed this May 30, 2023

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

phohensee deleted the 8304074 branch May 30, 2023 14:47

Conversation

phohensee commented May 4, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented May 4, 2023

Uh oh!

openjdk bot commented May 4, 2023

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

simonis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phohensee commented May 24, 2023

Uh oh!

dholmes-ora commented May 25, 2023

Uh oh!

phohensee commented May 25, 2023

Uh oh!

dholmes-ora commented May 26, 2023

Uh oh!

dholmes-ora commented May 26, 2023

Uh oh!

dholmes-ora commented May 26, 2023

Uh oh!

phohensee commented May 26, 2023

Uh oh!

mlchung commented May 26, 2023

Uh oh!

phohensee commented May 26, 2023

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora May 29, 2023

Choose a reason for hiding this comment

Uh oh!

phohensee May 29, 2023

Choose a reason for hiding this comment

Uh oh!

phohensee commented May 29, 2023

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

phohensee commented May 30, 2023

Uh oh!

phohensee commented May 30, 2023

phohensee commented May 4, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 4, 2023 •

edited

Loading