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

8262185: G1: Prune collection set candidates early #2693

Closed

Conversation

tschatzl
Copy link
Contributor

@tschatzl tschatzl commented Feb 23, 2021

Hello,

can I have reviews for this change to the collection candidate selection procedure, moving the G1HeapWastePercent exclusion criteria right after candidate selection instead of at the end of mixed gcs?

This can save lots of memory with negligible other impact.

Long version:
Currently G1 maintains collection set candidates from the marking phase (where it determines those) until the end of the mixed gc phase.

Mixed gc phase ends when the amount of space that can be reclaimed in the java heap with the remaining collection set candidates is smaller than G1HeapWastePercent of the (current) heap capacity.

This means that in some cases a significant amount of memory is used for regions that will never be evacuated. In addition to that, maintaining the remembered sets for these never evacuated regions uses execution time and more memory for the remembered set.

In fact, in some cases, it can happen that in the first few mixed gcs of a mixed gc phase, remembered set memory consumption increases even though the remembered sets of recently evacuated old gen regions are freed.

The proposed alternative is to prune the collection set candidates as early as possible, filtering out regions that are never going to be evacuated (or have a very low probability).

In some cases doing this can save half of peak remembered set memory usage.

There are a few drawbacks here that should be considered during evaluation, also comparing against the old heuristic.

  • In the old heuristic, G1 checks at the end of gc whether the remaining collection set candidates are worth collecting (remaining collectible space < G1HeapWastePercent means: stop). Which helps with ensuring at least some forward progress in evacuating the heap because (assuming there are candidates) at least some space will be reclaimed.
    (I do not know whether this behavior as is is intentional for that reason, but it has been there since initial implementation; then it did not really matter because G1 has been maintaining the old gen remembered sets for all regions all the time anyway).
    This is approximated by not removing all of the candidates to have at least one "minimal" mixed collection in this change.

  • In some cases in the old heuristic G1 would just evacuate all candidate regions (in the extreme in a single gc) if the pause time permitted, reclaiming a bit more space (i.e. that amount < G1HeapWastePercent of the total heap).
    You would expect (given the default value of 5), there will be more mixed cycles because of that with the new heuristic.

Testing: tier1-5, Oracle internal performance test suite

Thanks,
Thomas


Progress

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

Issue

  • JDK-8262185: G1: Prune collection set candidates early

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2693/head:pull/2693
$ git checkout pull/2693

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 23, 2021

👋 Welcome back tschatzl! 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
Copy link

openjdk bot commented Feb 23, 2021

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

  • hotspot-gc

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-gc hotspot-gc-dev@openjdk.org label Feb 23, 2021
@tschatzl tschatzl marked this pull request as ready for review February 24, 2021 08:32
@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 24, 2021
@mlbridge
Copy link

mlbridge bot commented Feb 24, 2021

Copy link
Member

@walulyai walulyai left a comment

Choose a reason for hiding this comment

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

Lgtm!

@tschatzl
Copy link
Contributor Author

@walulyai and me privately discussed some minor naming changes that the most recent push adds since I think they are good.

Also tried to improve the comment for G1CollectionSetCandidates::prune() a bit.

Copy link
Member

@walulyai walulyai left a comment

Choose a reason for hiding this comment

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

still good!

Copy link
Contributor

@kstefanj kstefanj left a comment

Choose a reason for hiding this comment

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

Looks good, just a few small comments.

src/hotspot/share/gc/g1/g1CollectionSetCandidates.cpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1CollectionSetCandidates.cpp Outdated Show resolved Hide resolved
Co-authored-by: Stefan Johansson <54407259+kstefanj@users.noreply.github.com>
Copy link
Contributor

@kstefanj kstefanj left a comment

Choose a reason for hiding this comment

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

Looks good =)

@openjdk
Copy link

openjdk bot commented Feb 24, 2021

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

8262185: G1: Prune collection set candidates early

Reviewed-by: iwalulya, sjohanss, 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 61 new commits pushed to the master branch:

  • 20c93b3: 8261914: IfNode::fold_compares_helper faces non-canonicalized bool when running JRuby JSON workload
  • ddd550a: 8261308: C2: assert(inner->is_valid_counted_loop(T_INT) && inner->is_strip_mined()) failed: OuterStripMinedLoop should have been removed
  • 03d888f: 8261804: Remove field _processing_is_mt, calculate it instead
  • 6800ba4: 8257500: Drawing MultiResolutionImage with ImageObserver "leaks" memory
  • 65a245e: 8262329: Fix JFR parser exception messages
  • a4c2496: 8259535: ECDSA SignatureValue do not always have the specified length
  • 2515c42: 8262332: serviceability/sa/ClhsdbJhisto.java fails with Test ERROR java.lang.RuntimeException: 'ParselTongue' missing from stdout/stderr
  • 07061fc: 8256417: Exclude TestJFRWithJMX test from running with PodMan
  • c9e9189: 8262074: Consolidate the default value of MetaspaceSize
  • 05c11bc: 8262426: Change TRAPS to Thread* for find_constrained_instance_or_array_klass()
  • ... and 51 more: https://git.openjdk.java.net/jdk/compare/9d9bedd051c313cf0f4552c6486c3f43bdaa81b9...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 Pull request is ready to be integrated label Feb 24, 2021
@tschatzl
Copy link
Contributor Author

Albert and me were discussing some further code improvements that this last change implements.

@tschatzl
Copy link
Contributor Author

... and then @kstefanj also chimed in and we thought of moving the logic completely away from G1CollectionSetCandidates which is in spirit of the current implementation.

tier1 tested again, local testing that it works.

Copy link
Contributor

@kstefanj kstefanj left a comment

Choose a reason for hiding this comment

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

Looks good, apart from the now unused code.

src/hotspot/share/gc/g1/g1CollectionSetCandidates.cpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp Outdated Show resolved Hide resolved
@tschatzl
Copy link
Contributor Author

Thanks, removed the obsolete code.

Copy link
Contributor

@kstefanj kstefanj left a comment

Choose a reason for hiding this comment

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

Still good.

@tschatzl
Copy link
Contributor Author

tschatzl commented Mar 1, 2021

Thanks @albertnetymk @kstefanj @walulyai for your reviews.

/integrate

@openjdk openjdk bot closed this Mar 1, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 1, 2021
@openjdk
Copy link

openjdk bot commented Mar 1, 2021

@tschatzl Since your change was applied there have been 62 commits pushed to the master branch:

  • 8bc8542: 8262195: Harden tests that use the HostsFileNameService (jdk.net.hosts.file property)
  • 20c93b3: 8261914: IfNode::fold_compares_helper faces non-canonicalized bool when running JRuby JSON workload
  • ddd550a: 8261308: C2: assert(inner->is_valid_counted_loop(T_INT) && inner->is_strip_mined()) failed: OuterStripMinedLoop should have been removed
  • 03d888f: 8261804: Remove field _processing_is_mt, calculate it instead
  • 6800ba4: 8257500: Drawing MultiResolutionImage with ImageObserver "leaks" memory
  • 65a245e: 8262329: Fix JFR parser exception messages
  • a4c2496: 8259535: ECDSA SignatureValue do not always have the specified length
  • 2515c42: 8262332: serviceability/sa/ClhsdbJhisto.java fails with Test ERROR java.lang.RuntimeException: 'ParselTongue' missing from stdout/stderr
  • 07061fc: 8256417: Exclude TestJFRWithJMX test from running with PodMan
  • c9e9189: 8262074: Consolidate the default value of MetaspaceSize
  • ... and 52 more: https://git.openjdk.java.net/jdk/compare/9d9bedd051c313cf0f4552c6486c3f43bdaa81b9...master

Your commit was automatically rebased without conflicts.

Pushed as commit 702ca62.

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

@tschatzl tschatzl deleted the 8262185-prune-remembered-sets-early branch March 12, 2021 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc hotspot-gc-dev@openjdk.org integrated Pull request has been integrated
4 participants