8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM

[phohensee]

I'd like to backport the definition and implementation of com.sun.management.ThreadMXBean.getTotalThreadAllocatedBytes to 8u. The backport CSR is JDK-8320375. A follow-up bugfix backport of JDK-8313081 will be done following this backport. The combined backports have been in production at Amazon for two months with no issues. The backport uses the reserved1 slot in jmm.h in order to preserve binary compatibility with 8u. Per current policy, there is no update to JMM_VERSION in jmm.h and the new method is marked

@since 8u412

Aside from file relocation and context differences, relative to the 11u backport the MonitoringSupport_lock definition macro changes, and the reserved1 rather than reserved6 jmm slot is used. SMR doesn't exist in 8u, so jmm_GetTotalThreadAllocated bytes attempts to lock Threads_lock instead, and if the Threads_lock is already locked, the previous return value is returned. HotSpotThreadImpl.java doesn't exist in 8u, so the hunk associated with it is dropped.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change requires CSR request JDK-8320375 to be approved
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• JDK-8304074 needs maintainer approval

Issues

• JDK-8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM (Enhancement - P4 - Approved)
• JDK-8320375: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM (CSR)

Reviewers

• Volker Simonis (@simonis - Reviewer)
• Andrew John Hughes (@gnu-andrew - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/392/head:pull/392
$ git checkout pull/392

Update a local copy of the PR:
$ git checkout pull/392
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/392/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 392

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/392.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 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]

This backport pull request has now been updated with issue from the original commit.

[mlbridge]

Webrevs

• 02: Full - Incremental (5d1c1cfa)
• 01: Full - Incremental (137de84e)
• 00: Full (50dc3e3b)

[simonis]

I didn't read the PR comment carefully (my fault :) so here's my list of changes with regards to the corresponding jdk11 change set (see openjdk/jdk11u-dev#2286). I'll provide them here in the hope to simplify the review for others:

• in jdk8 we have two jmm.h files, one in jdk/src/share/javavm/export/jmm.h and one in hotspot/src/share/vm/services/jmm.h. The changes to both files are the same.
• There's no HotSpotThreadImpl.java so the function added in the base class ThreadImpl.java has to be public in jdk8u.
• there is no jdk/make/mapfiles/libmanagement/mapfile-vers in jdk11u and later.
• there's no JavaThreadIteratorWithHandle/ThreadsListHandle in jdk8 that's why we have to use the Threads_lock while iterating over the threads. It's also the reason why parts of the comments in jmm_GetTotalThreadAllocatedMemory() became obsolete and have been removed.

@phohensee, is my assumption correct that you also use this already in production within Amazon and haven't seen any negative impact (particularly not due to the usage of the Threads_lock in jmm_GetTotalThreadAllocatedMemory() ?

[phohensee]

linux-x86 test failure is unrelated and has appeared in multiple PRs.

[phohensee]

Thanks for the quick review, Volker! Re your comments:

Yes, there are two jmm.h files with identical changes. I missed pointing that out.
HotSpotThreadImpl.java was mentioned in the PR comment.
Yes, there is no jdk/make/mapfiles/libmanagement/mapfile-vers in jdk11u and later. I missed pointing that out too.
Lack of SMR was mentioned in the PR comment.

The patch is indeed in production at Amazon and we've seen no impact from using the Threads_lock. There are several other methods in management.cpp that take the Threads_lock, so it's not an unusual use.

[simonis]

Thanks for the addition information Paul. Looks good now.

[phohensee]

Pre-submit failures appear unrelated.

[phohensee]

/approval request Backport of a purely additive JMX feature (doesn't change existing code). Backport required code changes due to 11u code restructuring and the lack of Thread SMR in 8u, but the changes are confined to additive or additive support code. In production at Amazon for over two months.

[phohensee]

The backport CSR has been approved, but the bot doesn't recognize it as being associated with the 8u backport.

[openjdk]

@phohensee
8304074: The approval request has been created successfully.

[bridgekeeper]

@phohensee This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[phohensee]

/open

[openjdk]

@phohensee This pull request is already open

[bridgekeeper]

@phohensee This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[phohensee]

/open

[openjdk]

@phohensee This pull request is already open

[gnu-andrew]

Thanks @simonis for your comments. It made these easier to review. Changes from 11u look fine.

@phohensee where is the 8u CSR? The only one I can find from the bug is the 21u CSR JDK-8307396 and there seems to be no 8u backport issue.

Given we are a week out from rampdown for 8u412, and this change has not yet been released in 11u & 17u, I would suggest we integrate this after rampdown for 8u422.

[phohensee]

@gun-andrew, @jerboaa suggested the same and I'm fine with 8u422.

I've created an 8u backport issue JDK-8326086. It can use the same CSR as used for 11u and 17u. How do I link a backport JBS issue to a CSR without creating a new one?

[gnu-andrew]

@gun-andrew, @jerboaa suggested the same and I'm fine with 8u422.

Ah ok, I didn't see any comments from him on here.

I've created an 8u backport issue JDK-8326086. It can use the same CSR as used for 11u and 17u. How do I link a backport JBS issue to a CSR without creating a new one?

I linked the CSR JDK-8320375 to the backport bug using the 'csr for' link, which is the same way it's connected to the 17u one. Let's see if it picks that up or not.

[gnu-andrew]

8u422 now open for commits
/approve yes

[openjdk]

@gnu-andrew
8304074: The approval request has been approved.

[openjdk]

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

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

8304074: [JMX] Add an approximation of total bytes allocated on the Java heap by the JVM

Reviewed-by: simonis, andrew

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

[gnu-andrew]

@gun-andrew, @jerboaa suggested the same and I'm fine with 8u422.

Ah ok, I didn't see any comments from him on here.

I've created an 8u backport issue JDK-8326086. It can use the same CSR as used for 11u and 17u. How do I link a backport JBS issue to a CSR without creating a new one?

I linked the CSR JDK-8320375 to the backport bug using the 'csr for' link, which is the same way it's connected to the 17u one. Let's see if it picks that up or not.

So the CSR needed to be linked to the backport issue and explicitly have 'openjdk8u422' as a version. I think the latter may be worth a Skara bug, as '8-pool' should be sufficient.

[phohensee]

/integrate

GHA failure is unrelated: passes on my development box.

[openjdk]

Going to push as commit 1fad52d.

[openjdk]

@phohensee Pushed as commit 1fad52d.

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