Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8278282: G1: Log basic statistics for evacuation failure #6860

Closed

Conversation

Hamlin-Li
Copy link

@Hamlin-Li Hamlin-Li commented Dec 16, 2021

The original pr is at #6763 , which should be retired as we have decided to adjust part of optimization solution for evacuation failure (see #6627 for details), so the log will be adjusted accordiingly.
The basic log related to evacuation failed will looks like below based on this patch.

[13.126s][debug][gc,phases] GC(0)       Restore Retained Regions (ms): Min:  0.0, Avg: 197.4, Max: 1579.1, Diff: 1579.1, Sum: 1579.1, Workers: 8
[13.126s][debug][gc,phases] GC(0)         Evacuation Failure Regions:    Min: 1, Avg:  1.0, Max: 1, Diff: 0, Sum: 1, Workers: 1

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8278282: G1: Log basic statistics for evacuation failure

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6860/head:pull/6860
$ git checkout pull/6860

Update a local copy of the PR:
$ git checkout pull/6860
$ git pull https://git.openjdk.java.net/jdk pull/6860/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6860

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6860.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 16, 2021

👋 Welcome back mli! 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 openjdk bot added the rfr label Dec 16, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 16, 2021

@Hamlin-Li 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.

@openjdk openjdk bot added the hotspot label Dec 16, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 16, 2021

Webrevs

Copy link
Contributor

@tschatzl tschatzl left a comment

Lgtm apart from the omission of that log message check.

I assume that it's just not worth adding log messages for the sub phases (sorting, ...) of the evacuation failure handling since we decided on the other option using the prev bitmap already. That's fine.
In any case we can always improve these messages.

@@ -265,7 +265,7 @@ private void testConcurrentRefinementLogs() throws Exception {
LogMessageWithLevel exhFailureMessages[] = new LogMessageWithLevel[] {
new LogMessageWithLevel("Recalculate Used Memory", Level.DEBUG),
new LogMessageWithLevel("Restore Preserved Marks", Level.DEBUG),
new LogMessageWithLevel("Remove Self Forwards", Level.DEBUG),
new LogMessageWithLevel("Restore Retained Regions", Level.DEBUG),
Copy link
Contributor

@tschatzl tschatzl Dec 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also check the work item in this test. Needs to be added just here.

Copy link
Author

@Hamlin-Li Hamlin-Li Dec 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Thomas, I have added checking newly added work items.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 17, 2021

@Hamlin-Li 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:

8278282: G1: Log basic statistics for evacuation failure

Reviewed-by: tschatzl, ayang, iwalulya

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

  • 09cf5f1: 8278602: CDS dynamic dump may access unloaded classes
  • 8dc4437: 8278434: timeouts in test java/time/test/java/time/format/TestZoneTextPrinterParser.java
  • 6b906bb: 8279223: Define version in .jcheck/conf
  • c295e71: 8276700: Improve java.lang.ref.Cleaner javadocs
  • 3a1fca3: 8278146: G1: Rework VM_G1Concurrent VMOp to clearly identify it as pause
  • 2a59ebb: 8279119: src/jdk.hotspot.agent/doc/index.html file contains references to scripts that no longer exist
  • 299022d: 8279225: [arm32] C1 longs comparison operation destroys argument registers
  • 4f607f2: Merge
  • 54b800d: 8271202: C1: assert(false) failed: live_in set of first block must be empty
  • 2945b78: 8279195: Document the -XX:+NeverActAsServerClassMachine flag
  • ... and 34 more: https://git.openjdk.java.net/jdk/compare/bf2826499a26363d01a5269bd2f54e9c363d4cdc...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 openjdk bot added the ready label Dec 17, 2021
@Hamlin-Li Hamlin-Li changed the title 8278282: G1: Log basic statistics of evacuation failure 8278282: G1: Log basic statistics for evacuation failure Dec 22, 2021
@Hamlin-Li
Copy link
Author

@Hamlin-Li Hamlin-Li commented Dec 22, 2021

Can I have another reviewer?
Thanks

@Hamlin-Li
Copy link
Author

@Hamlin-Li Hamlin-Li commented Dec 23, 2021

/label add hotspot-gc

@openjdk openjdk bot added the hotspot-gc label Dec 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 23, 2021

@Hamlin-Li
The hotspot-gc label was successfully added.

@Hamlin-Li
Copy link
Author

@Hamlin-Li Hamlin-Li commented Dec 23, 2021

Can I have another reviewer?
Thanks

Copy link
Member

@albertnetymk albertnetymk left a comment

Just a minor comment.

@@ -132,6 +132,8 @@ G1GCPhaseTimes::G1GCPhaseTimes(STWGCTimer* gc_timer, uint max_gc_threads) :
_gc_par_phases[MergePSS]->create_thread_work_items("LAB Waste", MergePSSLABWasteBytes);
_gc_par_phases[MergePSS]->create_thread_work_items("LAB Undo Waste", MergePSSLABUndoWasteBytes);

_gc_par_phases[RestoreRetainedRegions]->create_thread_work_items("Evacuation Failure Regions:", RestoreRetainedRegionsNum);
Copy link
Member

@albertnetymk albertnetymk Jan 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the surrounding naming conventions, RestoreRetainedRegionsNum should probably be sth like RestoreRetainedRegionsNumEvacFailRegions or just the suffix NumEvacFailRegions.

Copy link
Author

@Hamlin-Li Hamlin-Li Jan 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Albert, I see your point.
But seems the surrounding naming convention is not that strict, and there is no uniform convention here. And to me, RestoreRetainedRegionsNum is more friendly to read and easily related to RestoreRetainedRegions, so I prefer to keep it as RestoreRetainedRegionsNum.

@Hamlin-Li
Copy link
Author

@Hamlin-Li Hamlin-Li commented Jan 4, 2022

Thanks @tschatzl @albertnetymk @walulyai for your reviews.

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jan 4, 2022

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

  • 1ffdc52: 8279412: [JVMCI] failed speculations list must outlive any nmethod that refers to it
  • 863bffb: 8279374: Remove unused JNIHandles::weak_oops_do
  • 9bdf6eb: 8279385: [test] Adjust sun/security/pkcs12/KeytoolOpensslInteropTest.java after 8278344
  • 09cf5f1: 8278602: CDS dynamic dump may access unloaded classes
  • 8dc4437: 8278434: timeouts in test java/time/test/java/time/format/TestZoneTextPrinterParser.java
  • 6b906bb: 8279223: Define version in .jcheck/conf
  • c295e71: 8276700: Improve java.lang.ref.Cleaner javadocs
  • 3a1fca3: 8278146: G1: Rework VM_G1Concurrent VMOp to clearly identify it as pause
  • 2a59ebb: 8279119: src/jdk.hotspot.agent/doc/index.html file contains references to scripts that no longer exist
  • 299022d: 8279225: [arm32] C1 longs comparison operation destroys argument registers
  • ... and 37 more: https://git.openjdk.java.net/jdk/compare/bf2826499a26363d01a5269bd2f54e9c363d4cdc...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Jan 4, 2022
@openjdk openjdk bot closed this Jan 4, 2022
@openjdk openjdk bot removed ready rfr labels Jan 4, 2022
@openjdk
Copy link

@openjdk openjdk bot commented Jan 4, 2022

@Hamlin-Li Pushed as commit 93c7d90.

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

@Hamlin-Li Hamlin-Li deleted the log-evac-failure-region-num branch Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-gc integrated
4 participants