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

8311602 GenShen: Decouple generational mode heuristics #292

Conversation

earthling-amzn
Copy link
Contributor

@earthling-amzn earthling-amzn commented Jun 29, 2023

The general idea here is to straighten out code paths which fork based on the mode configuration. A ShenandoahYoungHeuristics class has been added to host the logic specific to the generational mode. In many cases, the entangled generational code can simply be moved behind methods which override existing virtual methods.

A few other notable changes:

  • The confusing "trigger" heuristic has been removed from ShenandoahOldHeuristics.
    • The old heuristic defines its own triggers now. It is no longer possible to specify the old generation's trigger heuristic.
  • Selection of aged regions for in-place promotion has been moved to ShenandoahGeneration.
    • The method implementation does not depend on any members of ShenandoahHeuristics. Moving the method therefore removes an unnecessary dependency on the heuristics.
  • A new ShenandoahHeapCharacteristics interface has been added to provide information about the heap or generation to the heuristics.
    • On the development branch, this interface is implemented by ShenandoahGeneration.
    • The plan is to upstream this interface and have it implemented by ShenandoahHeap

The taxonomy of these heuristic classes has become somewhat more complex - here is a chart showing the relationships:

ShenandoahHeuristics
-  ShenandoahPassiveHeuristics
-  ShenandoahCompactHeuristics
-  ShenandoahAggressiveHeuristics
-  ShenandoahStaticHeuristics
-  ShenandoahOldHeuristics
+  ShenandoahAdaptiveHeuristics
  +  ShenandoahGenerationalHeuristics
    -  ShenandoahYoungHeuristics
    -  ShenandoahGlobalHeuristics       

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-8311602: GenShen: Decouple generational mode heuristics (Task - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 292

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 29, 2023

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

@earthling-amzn earthling-amzn changed the title Decouple generational heuristics 8311602 GenShen: Decouple generational mode heuristics Jul 6, 2023
@earthling-amzn earthling-amzn marked this pull request as ready for review July 7, 2023 23:09
@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 7, 2023
@mlbridge
Copy link

mlbridge bot commented Jul 7, 2023

Copy link
Contributor

@kdnilsen kdnilsen left a comment

Choose a reason for hiding this comment

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

Thank you very much for working through all these details. Looks good. I assume we've compared performance pipeline results and see no regresions.

@openjdk
Copy link

openjdk bot commented Jul 10, 2023

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

8311602: GenShen: Decouple generational mode heuristics

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 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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jul 10, 2023
@kdnilsen
Copy link
Contributor

Looks like the Windows x86 tier1 test failures needs to be resolved still.

@earthling-amzn
Copy link
Contributor Author

Looks like the Windows x86 tier1 test failures needs to be resolved still.

This failure exists on mainline, I don't think it's related to the changes here: https://bugs.openjdk.org/browse/JDK-8311843

@kdnilsen
Copy link
Contributor

Ok. That assert can be addressed under the other ticket.

@@ -755,8 +950,10 @@ void ShenandoahGeneration::decrease_capacity(size_t decrement) {
}

void ShenandoahGeneration::record_success_concurrent(bool abbreviated) {
ShenandoahHeap* heap = ShenandoahHeap::heap();
abbreviated = abbreviated && heap->mode()->is_generational();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to re-evaluate performance of this change on diluvian benchmark

Copy link
Member

@ysramakrishna ysramakrishna left a comment

Choose a reason for hiding this comment

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

Thanks for the code walk through which allowed me to understand the scope and objective of the changes much better. This generally looks good to me, and greatly cleans up the code changes that previously stemmed from the "new" generational mode.

  • I left a few questions/comments in-line in a few places that caught my eye or where things were not clear to me.
  • Some of this will be in files/classes that are in upstream, which we'd like not to change, but I'd really like to see us take a stab at writing a 1 or 2-sentence header comment for each of the derived classes or ShenandoahHeuristics. Hopefully those additions can be upstreamed in your other PR (or will not raise issues when the GenShen PR is put up for review as the documentation comments are easy to see as having not made any code changes). It tends to make code reading much more comprehensible by someone looking at the code for the first time or after a long gap. :-) I've left a comment to similar effect specifically for the class ShenandoahOldHeuristic.

Rest looks good to me. Thanks for doing this refactoring & clean-up!

src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp Outdated Show resolved Hide resolved
//
// A second benefit of treating aged regions differently than other regions during collection set selection is
// that this allows us to more accurately budget memory to hold the results of evacuation. Memory for evacuation
// of aged regions must be reserved in the old generations. Memory for evacuation of all other regions must be
Copy link
Member

Choose a reason for hiding this comment

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

Memory for objects with age exceeding tenuring threshold could be in regions with age "0", correct? If that is so, the last sentence here may be slightly inaccurate. Or perhaps it will be inaccurate when adaptive tenuring is actually in place? (Although I'd imagine that it might be inaccurate even today with a fixed/static tenuring age.)

src/hotspot/share/gc/shenandoah/shenandoahGeneration.cpp Outdated Show resolved Hide resolved
@earthling-amzn
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Jul 20, 2023

Going to push as commit 09f6c03.
Since your change was applied there have been 3 commits pushed to the master branch:

  • 0fbfe19: 8312321: GenShen: Remembered set scan may encounter garbage objects
  • 514fd37: 8312422: GenShen: In-place region promotion state may carry over when evacuation fails
  • e754b19: 8312322: GenShen: Cancelled GCs may become stuck in self-cancellation loop

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jul 20, 2023
@openjdk openjdk bot closed this Jul 20, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 20, 2023
@openjdk
Copy link

openjdk bot commented Jul 20, 2023

@earthling-amzn Pushed as commit 09f6c03.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
3 participants