8319342: GenShen: Reset the count of degenerated cycles in a row following Full GC #352

kdnilsen · 2023-11-03T21:17:42Z

After a Full GC completes, reset the count on consecutive degenerated cycles to zero. This prevents automatic triggering of back-to-back Full GC.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

JDK-8319342: GenShen: Reset the count of degenerated cycles in a row following Full GC (Bug - P3)

Reviewers

William Kemper (@earthling-amzn - Committer)
Y. Srinivas Ramakrishna (@ysramakrishna - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 352

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

Using diff file

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

Webrev

Link to Webrev Comment

Also remove unused method in ShenandoahGeneration.

bridgekeeper · 2023-11-03T21:18:40Z

👋 Welcome back kdnilsen! 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-11-03T21:25:48Z

Webrevs

earthling-amzn · 2023-11-06T22:49:57Z

src/hotspot/share/gc/shenandoah/shenandoahCollectorPolicy.hpp

@@ -61,6 +61,8 @@ class ShenandoahCollectorPolicy : public CHeapObj<mtGC> {
  ShenandoahSharedFlag _in_shutdown;
  ShenandoahTracer* _tracer;

+  uint _degenerated_cycles_in_a_row;


Nit, but we could call this _consecutive_degenerated_gcs for consistency with other fields in this class? (Also, possibly rename _abbreviated_cycles to _abbreviated_gcs for the same reason.

Thanks. Making this change.

earthling-amzn · 2023-11-06T22:59:31Z

src/hotspot/share/gc/shenandoah/heuristics/shenandoahPassiveHeuristics.cpp

@@ -44,11 +44,6 @@ bool ShenandoahPassiveHeuristics::should_unload_classes() {
  return can_unload_classes();
 }

-bool ShenandoahPassiveHeuristics::should_degenerate_cycle() {
-  // Always fail to Degenerated GC, if enabled
-  return ShenandoahDegeneratedGC;


Are we changing how the passive heuristic behaves now? With this change ShenandoahCollectorPolicy::should_degenerate_cycle would return false sometimes when the passive heuristic would have "degenerated".

I agee with William... You can leave all state (counters) and its manipulation entirely in the heuristics object, and let the policy object query the heuristics like you do today, and inform it when it does do a full gc, etc. so relevant state may be updated.

Of course, the policy should not independently reach in and change the state maintained in the heuristics, just query that state, and inform it of events that might entail changes to that state.

We can clean things up by asking what the role of heuristics is vs what the role of policy is. It seems as if policy is broad and heuristics is more specific within that broad policy. It is also possible to keep the state in the policy, be mutated only by policy in response to events, and have the heuristics be completely state-free, but call-back to policy to use any state it would use for its decisions. Clarifying this structure and their relationship, and sticking to a consistent (documented) protocol here would avoid confusion and errors.

Good catch. That was an unintended consequence. I will return the methods to the Heuristics hierarchy, but will query the consecutive_degenerated_gcs value from collector policy.

Restore the original ShenandoahHeuristics::should_degenerate_cycle() so that ShenandoahPassiveHeuristics can differentate from other implementations. Improve instance variable names.

openjdk · 2023-11-13T17:07:16Z

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

8319342: GenShen: Reset the count of degenerated cycles in a row following Full GC

Reviewed-by: wkemper, 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 161 new commits pushed to the master branch:

2db0157: 8319931: GenShen: Increase no progress threshold for TestThreadFailure
e8602e7: 8319867: GenShen: Make old regions parsable at end of concurrent cycles
a9718c1: Merge
8555e0f: 8319318: bufferedStream fixed case can be removed
73c5f60: 8319556: Harmonize interface formatting in the FFM API
cc4b0d9: 8319378: Spec for j.util.Timer::purge and j.util.Timer::cancel could be improved
a290256: 8315680: java/lang/ref/ReachabilityFenceTest.java should run with -Xbatch
b1625af: 8305814: Update Xalan Java to 2.7.3
806529a: 8319573: Change to Visual Studio 17.6.5 for building on Windows at Oracle
e9eb8b9: 8319338: tools/jpackage/share/RuntimeImageTest.java fails with -XX:+UseZGC
... and 151 more: https://git.openjdk.org/shenandoah/compare/e4a8a43806537303fbf18a062ac536f0c2729551...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.

kdnilsen · 2023-11-13T21:22:20Z

/integrate

openjdk · 2023-11-13T21:23:31Z

Going to push as commit 81de88c.
Since your change was applied there have been 161 commits pushed to the master branch:

2db0157: 8319931: GenShen: Increase no progress threshold for TestThreadFailure
e8602e7: 8319867: GenShen: Make old regions parsable at end of concurrent cycles
a9718c1: Merge
8555e0f: 8319318: bufferedStream fixed case can be removed
73c5f60: 8319556: Harmonize interface formatting in the FFM API
cc4b0d9: 8319378: Spec for j.util.Timer::purge and j.util.Timer::cancel could be improved
a290256: 8315680: java/lang/ref/ReachabilityFenceTest.java should run with -Xbatch
b1625af: 8305814: Update Xalan Java to 2.7.3
806529a: 8319573: Change to Visual Studio 17.6.5 for building on Windows at Oracle
e9eb8b9: 8319338: tools/jpackage/share/RuntimeImageTest.java fails with -XX:+UseZGC
... and 151 more: https://git.openjdk.org/shenandoah/compare/e4a8a43806537303fbf18a062ac536f0c2729551...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-11-13T21:23:43Z

@kdnilsen Pushed as commit 81de88c.

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

kdnilsen added 2 commits November 3, 2023 00:50

Count degen cycles in a row within ShenandoahCollectorPolicy

bdbef70

Do not increment degen count if degen upgrades to full

3b3a167

Also remove unused method in ShenandoahGeneration.

openjdk bot added the rfr Pull request is ready for review label Nov 3, 2023

earthling-amzn suggested changes Nov 6, 2023

View reviewed changes

kdnilsen added 2 commits November 7, 2023 21:52

Respond to reviewer feedback

8bd0b99

Restore the original ShenandoahHeuristics::should_degenerate_cycle() so that ShenandoahPassiveHeuristics can differentate from other implementations. Improve instance variable names.

Fix typo

b18f704

earthling-amzn approved these changes Nov 13, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 13, 2023

ysramakrishna approved these changes Nov 13, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Nov 13, 2023

openjdk bot closed this Nov 13, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8319342: GenShen: Reset the count of degenerated cycles in a row following Full GC #352

8319342: GenShen: Reset the count of degenerated cycles in a row following Full GC #352

kdnilsen commented Nov 3, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 3, 2023

mlbridge bot commented Nov 3, 2023 •

edited

Loading

earthling-amzn Nov 6, 2023

kdnilsen Nov 7, 2023

earthling-amzn Nov 6, 2023

ysramakrishna Nov 7, 2023

kdnilsen Nov 7, 2023

openjdk bot commented Nov 13, 2023 •

edited

Loading

kdnilsen commented Nov 13, 2023

openjdk bot commented Nov 13, 2023

openjdk bot commented Nov 13, 2023

8319342: GenShen: Reset the count of degenerated cycles in a row following Full GC #352

8319342: GenShen: Reset the count of degenerated cycles in a row following Full GC #352

Conversation

kdnilsen commented Nov 3, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Nov 3, 2023

mlbridge bot commented Nov 3, 2023 • edited Loading

Webrevs

earthling-amzn Nov 6, 2023

Choose a reason for hiding this comment

kdnilsen Nov 7, 2023

Choose a reason for hiding this comment

earthling-amzn Nov 6, 2023

Choose a reason for hiding this comment

ysramakrishna Nov 7, 2023

Choose a reason for hiding this comment

kdnilsen Nov 7, 2023

Choose a reason for hiding this comment

openjdk bot commented Nov 13, 2023 • edited Loading

kdnilsen commented Nov 13, 2023

openjdk bot commented Nov 13, 2023

openjdk bot commented Nov 13, 2023

kdnilsen commented Nov 3, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 3, 2023 •

edited

Loading

openjdk bot commented Nov 13, 2023 •

edited

Loading