Use distinct "end of cycle" message for each Shenandoah pause #270

earthling-amzn · 2023-05-02T22:54:53Z

This change allows the GarbageCollectionNotificationInfo::gcAction to be overridden. Shenandoah now uses this to distinguish the notification raised for each of the pauses during the concurrent cycle. This will make it easier for monitoring software to identify which phases may be having longer than expected pauses.

Progress

Change must not contain extraneous whitespace
Change must be properly reviewed (1 review required, with at least 1 Committer)

Reviewers

Kelvin Nilsen (@kdnilsen - Committer)
Y. Srinivas Ramakrishna (@ysramakrishna - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 270

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/shenandoah/pull/270.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-02T22:56:08Z

👋 Welcome back wkemper! 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.

mlbridge · 2023-05-02T23:02:04Z

Webrevs

01: Full - Incremental (1c097462)
00: Full (088002da)

kdnilsen · 2023-05-02T23:04:03Z

src/hotspot/share/gc/shenandoah/shenandoahVMOperations.cpp

  _gc->entry_final_updaterefs();
 }

 void VM_ShenandoahFinalRoots::doit() {
-  ShenandoahGCPauseMark mark(_gc_id, SvcGCMarker::CONCURRENT);


Why are we disabling the aging of regions? And why are we disabling the aging cycle?

In my initial review, I missed that this functionality has been moved to ShenandoahConcurrentGC::entry_final_roots()

I moved it into ShenandoahConcurrentGC::op_final_roots to fit the pattern of other phases (VM_Operation_XYZ and entry_XYZ should just have GC tracking boiler plate). Also, with this approach, we don't have to marshal the is_aging_cycle around.

Thanks for clarifying.

As I think on this a bit more... we should probably also age regions during final update refs. The "final roots" phase only happens on abbreviated cycles.

Thanks for catching that. We need it in both places or some code path shared between the different cycles.

kdnilsen · 2023-05-02T23:12:16Z

src/hotspot/share/gc/shenandoah/shenandoahVMOperations.cpp

  _gc->entry_final_updaterefs();
 }

 void VM_ShenandoahFinalRoots::doit() {
-  ShenandoahGCPauseMark mark(_gc_id, SvcGCMarker::CONCURRENT);


In my initial review, I missed that this functionality has been moved to ShenandoahConcurrentGC::entry_final_roots()

kdnilsen · 2023-05-02T23:17:24Z

src/hotspot/share/gc/shenandoah/shenandoahVMOperations.cpp

  _gc->entry_final_updaterefs();
 }

 void VM_ShenandoahFinalRoots::doit() {
-  ShenandoahGCPauseMark mark(_gc_id, SvcGCMarker::CONCURRENT);


Thanks for clarifying.

openjdk · 2023-05-02T23:19:58Z

@earthling-amzn 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:

Use distinct "end of cycle" message for each Shenandoah pause

Reviewed-by: kdnilsen, ysr

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

4e1ad18: Remove the remaining unclean upstream difference
21982fc: Merge openjdk/jdk:master
750bece: 8305771: SA ClassWriter.java fails to skip overpass methods
b81c9c8: 8306951: [BACKOUT] JDK-8305252 make_method_handle_intrinsic may call java code under a lock
732179c: 8306409: Open source AWT KeyBoardFocusManger, LightWeightComponent related tests
38cc039: 8306705: com/sun/jdi/PopAndInvokeTest.java fails with NativeMethodException
01b8512: 8302182: Update Public Suffix List to 88467c9
8e36c05: 8305853: java/text/Format/DateFormat/DateFormatRegression.java fails with "Uncaught exception thrown in test method Test4089106"
d0e8aec: 8306374: (bf) Improve performance of DirectCharBuffer::append(CharSequence[,int,int])
a18191f: 8302328: [s390x] Simplify asm_assert definition
... and 198 more: https://git.openjdk.org/shenandoah/compare/58fa1410a8ad1462cbe7c035b7d64f44a0b62c85...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.

ysramakrishna

LGTM.

src/hotspot/share/gc/shenandoah/shenandoahVMOperations.cpp

src/hotspot/share/services/memoryManager.cpp

earthling-amzn · 2023-05-03T17:07:34Z

/integrate

openjdk · 2023-05-03T17:08:51Z

Going to push as commit 90a1c9b.
Since your change was applied there have been 208 commits pushed to the master branch:

4e1ad18: Remove the remaining unclean upstream difference
21982fc: Merge openjdk/jdk:master
750bece: 8305771: SA ClassWriter.java fails to skip overpass methods
b81c9c8: 8306951: [BACKOUT] JDK-8305252 make_method_handle_intrinsic may call java code under a lock
732179c: 8306409: Open source AWT KeyBoardFocusManger, LightWeightComponent related tests
38cc039: 8306705: com/sun/jdi/PopAndInvokeTest.java fails with NativeMethodException
01b8512: 8302182: Update Public Suffix List to 88467c9
8e36c05: 8305853: java/text/Format/DateFormat/DateFormatRegression.java fails with "Uncaught exception thrown in test method Test4089106"
d0e8aec: 8306374: (bf) Improve performance of DirectCharBuffer::append(CharSequence[,int,int])
a18191f: 8302328: [s390x] Simplify asm_assert definition
... and 198 more: https://git.openjdk.org/shenandoah/compare/58fa1410a8ad1462cbe7c035b7d64f44a0b62c85...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-03T17:09:05Z

@earthling-amzn Pushed as commit 90a1c9b.

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

earthling-amzn added 2 commits May 2, 2023 15:39

Allow unique message for each gc notification action

092d94d

Simplify region aging logic

088002d

openjdk bot added the rfr label May 2, 2023

kdnilsen reviewed May 2, 2023

View reviewed changes

Move region aging into op_final_roots

1c09746

kdnilsen approved these changes May 2, 2023

View reviewed changes

openjdk bot added the ready label May 2, 2023

ysramakrishna approved these changes May 3, 2023

View reviewed changes

src/hotspot/share/gc/shenandoah/shenandoahVMOperations.cpp Show resolved Hide resolved

src/hotspot/share/services/memoryManager.cpp Show resolved Hide resolved

openjdk bot added the integrated label May 3, 2023

openjdk bot closed this May 3, 2023

openjdk bot removed ready rfr labels May 3, 2023

earthling-amzn deleted the distinguish-phase-actions branch May 4, 2023 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use distinct "end of cycle" message for each Shenandoah pause #270

Use distinct "end of cycle" message for each Shenandoah pause #270

earthling-amzn commented May 2, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 2, 2023

mlbridge bot commented May 2, 2023 •

edited

Loading

kdnilsen May 2, 2023

kdnilsen May 2, 2023

earthling-amzn May 2, 2023

kdnilsen May 2, 2023

earthling-amzn May 2, 2023

kdnilsen May 2, 2023

kdnilsen May 2, 2023

kdnilsen May 2, 2023

openjdk bot commented May 2, 2023 •

edited

Loading

ysramakrishna left a comment

earthling-amzn commented May 3, 2023

openjdk bot commented May 3, 2023

openjdk bot commented May 3, 2023

Use distinct "end of cycle" message for each Shenandoah pause #270

Use distinct "end of cycle" message for each Shenandoah pause #270

Conversation

earthling-amzn commented May 2, 2023 • edited by openjdk bot Loading

Progress

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 2, 2023

mlbridge bot commented May 2, 2023 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented May 2, 2023 • edited Loading

ysramakrishna left a comment

Choose a reason for hiding this comment

earthling-amzn commented May 3, 2023

openjdk bot commented May 3, 2023

openjdk bot commented May 3, 2023

earthling-amzn commented May 2, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 2, 2023 •

edited

Loading

openjdk bot commented May 2, 2023 •

edited

Loading