8292265: Add old gen used field at G1HeapSummary JFR event #11303

sangheon · 2022-11-22T23:09:52Z

Hi all,

Can I have reviews for this change which adds 'old gen used' field at existing G1HeapSummary event?

Testing: tier1 ~ tier3

Thanks,
Sangheon

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-8292265: Add old gen used field at G1HeapSummary JFR event

Reviewers

Thomas Schatzl (@tschatzl - Reviewer) ⚠️ Review applies to 85a4ac3a
Albert Mingkun Yang (@albertnetymk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11303

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

Using diff file

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

bridgekeeper · 2022-11-22T23:11:15Z

👋 Welcome back sangheki! 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 · 2022-11-22T23:14:22Z

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

hotspot

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.

sangheon · 2022-11-22T23:15:02Z

/label add hotspot-gc, hotspot-jfr
/label remove hotspot

openjdk · 2022-11-22T23:16:25Z

@sangheon
The hotspot-gc label was successfully added.

The hotspot-jfr label was successfully added.

openjdk · 2022-11-22T23:18:00Z

@sangheon
The hotspot label was successfully removed.

mlbridge · 2022-11-23T02:26:46Z

Webrevs

tschatzl · 2022-11-23T18:13:01Z

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

+  size_t old_used_bytes = _monitoring_support->old_gen_used();
+


On the one hand using the information from the MXBeans/MonitoringSupport and on the other hand recalculating this data manually, is likely to set us up for inconsistencies.
I recommend using MonitoringSupport data for as many as possible values here.

From what I can see for young gc, this method is called as part of the G1JFRTracerMark which encloses the G1MonitoringScope, i.e. the data from MonitoringSupport is all up to date. Same seems to be true for full gc.

So getting all data from MonitoringSupport here is imo the best way to get consistent data vs. using parts of that data and recalculating others manually.

tschatzl

Lgtm.

openjdk · 2022-11-30T09:24:47Z

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

8292265: Add old gen used field at G1HeapSummary JFR event

Reviewed-by: tschatzl, ayang

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

478ef38: 8296812: sprintf is deprecated in Xcode 14
715bf70: Merge
a894649: 8298225: [AIX] Disable PPC64LE continuations on AIX
52fffdd: 8298463: tools/javac/modules/EdgeCases.java fails on Windows after JDK-8297988
b7b996c: 8298353: C2 fails with assert(opaq->outcnt() == 1 && opaq->in(1) == limit) failed
05d67f6: 8298340: java/net/httpclient/CancelRequestTest.java fails with AssertionError: Found some subscribers for testPostInterrupt
5a92bee: 8298455: JFR: Add logging to TestClose.java
133ad8e: 8297988: NPE in JavacTypes.getOverriddenMethods from doclint
553ba65: 8298402: ProblemList javax/swing/JFileChooser/4847375/bug4847375.java on windows-x64
9749a43: 8298457: Instructions in a11y manual tests need to be updated
... and 373 more: https://git.openjdk.org/jdk/compare/373e52c0ab0d4fd3c6b18e67e0c46d1d1f0ac91e...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.

albertnetymk · 2022-12-05T14:37:30Z

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

-  size_t survivor_used_bytes = _survivor.used_bytes();
+  size_t eden_used_bytes = _monitoring_support->eden_space_used();
+  size_t survivor_used_bytes = _monitoring_support->survivor_space_used();
+  size_t old_used_bytes = _monitoring_support->old_gen_used();


I think it's best to include gen in the name (across the whole PR), just to avoid the confusion of Old-region-type vs old-generation.

sangheon · 2022-12-08T22:12:38Z

Thanks @tschatzl and @albertnetymk for your reviews.

/integrate

openjdk · 2022-12-08T22:15:25Z

@sangheon This pull request has not yet been marked as ready for integration.

sangheon · 2022-12-09T23:30:06Z

/integrate

openjdk · 2022-12-09T23:31:26Z

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

478ef38: 8296812: sprintf is deprecated in Xcode 14
715bf70: Merge
a894649: 8298225: [AIX] Disable PPC64LE continuations on AIX
52fffdd: 8298463: tools/javac/modules/EdgeCases.java fails on Windows after JDK-8297988
b7b996c: 8298353: C2 fails with assert(opaq->outcnt() == 1 && opaq->in(1) == limit) failed
05d67f6: 8298340: java/net/httpclient/CancelRequestTest.java fails with AssertionError: Found some subscribers for testPostInterrupt
5a92bee: 8298455: JFR: Add logging to TestClose.java
133ad8e: 8297988: NPE in JavacTypes.getOverriddenMethods from doclint
553ba65: 8298402: ProblemList javax/swing/JFileChooser/4847375/bug4847375.java on windows-x64
9749a43: 8298457: Instructions in a11y manual tests need to be updated
... and 373 more: https://git.openjdk.org/jdk/compare/373e52c0ab0d4fd3c6b18e67e0c46d1d1f0ac91e...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-12-09T23:31:46Z

@sangheon Pushed as commit 8ea2a67.

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

8292265: Add old gen used field at G1HeapSummary JFR event

5c8c1d3

openjdk bot changed the title ~~8292265: Add old gen used field at G1HeapSummary JFR event~~ 8292265: Add old gen used field at G1HeapSummary JFR event. Nov 22, 2022

openjdk bot added the hotspot hotspot-dev@openjdk.org label Nov 22, 2022

openjdk bot added hotspot-gc hotspot-gc-dev@openjdk.org hotspot-jfr hotspot-jfr-dev@openjdk.org labels Nov 22, 2022

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Nov 22, 2022

sangheon marked this pull request as ready for review November 23, 2022 02:20

openjdk bot added the rfr Pull request is ready for review label Nov 23, 2022

tschatzl suggested changes Nov 23, 2022

View reviewed changes

Thomas review

85a4ac3

sangheon requested a review from tschatzl November 29, 2022 07:13

tschatzl approved these changes Nov 30, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 30, 2022

albertnetymk reviewed Dec 5, 2022

View reviewed changes

Albert review

d538cbe

openjdk bot removed the ready Pull request is ready to be integrated label Dec 8, 2022

albertnetymk approved these changes Dec 8, 2022

View reviewed changes

sangheon changed the title ~~8292265: Add old gen used field at G1HeapSummary JFR event.~~ 8292265: Add old gen used field at G1HeapSummary JFR event Dec 9, 2022

openjdk bot added the ready Pull request is ready to be integrated label Dec 9, 2022

openjdk bot added the integrated Pull request has been integrated label Dec 9, 2022

openjdk bot closed this Dec 9, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8292265: Add old gen used field at G1HeapSummary JFR event #11303

8292265: Add old gen used field at G1HeapSummary JFR event #11303

sangheon commented Nov 22, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 22, 2022

openjdk bot commented Nov 22, 2022

sangheon commented Nov 22, 2022

openjdk bot commented Nov 22, 2022

openjdk bot commented Nov 22, 2022

mlbridge bot commented Nov 23, 2022 •

edited

Loading

tschatzl Nov 23, 2022 •

edited

Loading

tschatzl left a comment

openjdk bot commented Nov 30, 2022 •

edited

Loading

albertnetymk Dec 5, 2022

sangheon commented Dec 8, 2022

openjdk bot commented Dec 8, 2022

sangheon commented Dec 9, 2022

openjdk bot commented Dec 9, 2022

openjdk bot commented Dec 9, 2022

		size_t old_used_bytes = _monitoring_support->old_gen_used();

8292265: Add old gen used field at G1HeapSummary JFR event #11303

8292265: Add old gen used field at G1HeapSummary JFR event #11303

Conversation

sangheon commented Nov 22, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 22, 2022

openjdk bot commented Nov 22, 2022

sangheon commented Nov 22, 2022

openjdk bot commented Nov 22, 2022

openjdk bot commented Nov 22, 2022

mlbridge bot commented Nov 23, 2022 • edited Loading

Webrevs

tschatzl Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

tschatzl left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 30, 2022 • edited Loading

albertnetymk Dec 5, 2022

Choose a reason for hiding this comment

sangheon commented Dec 8, 2022

openjdk bot commented Dec 8, 2022

sangheon commented Dec 9, 2022

openjdk bot commented Dec 9, 2022

openjdk bot commented Dec 9, 2022

sangheon commented Nov 22, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 23, 2022 •

edited

Loading

tschatzl Nov 23, 2022 •

edited

Loading

openjdk bot commented Nov 30, 2022 •

edited

Loading