8325673: GenShen: Share Reserves between Old and Young Collector #395

kdnilsen · 2024-02-12T18:22:42Z

Allow young-gen Collector reserve to share memory with old-gen Collector reserve in order to support prompt processing of mixed evacuations, as constrained by ShenandoahOldEvacRatioPercent.

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-8325673: GenShen: Share Reserves between Old and Young Collector (Bug - P5)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 395

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-02-12T18:23: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 · 2024-02-12T18:27:23Z

Webrevs

Given that mixed evacuation will sometimes borrow up to this amount of memory from the young evacuation reserve for old evacuations, we reduce the default value to reduce the impact on normal young GC.

1. Always require that evacuation reserve quantities be established before rebuilding free set 2. Establish evacuation reserve quantities before initial rebuild of free set 3. Fix the calucation of old reserves during adjust generation sizes for next gc cycle

This reverts commit 7bb1d38.

When we round up, we introduce the risk that the new size exceeds the maximum LAB size, resulting in an assertion error.

This reverts commit 99cce53.

In the previous implementaiton, reserve quantities were only sometimes established.

openjdk · 2024-02-27T22:00:21Z

@kdnilsen this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout share-collector-reserves
git fetch https://git.openjdk.org/shenandoah.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2024-03-13T20:01:37Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

This reverts commit d881300.

…erves

Limit the size of old-gen by memory available in the OldCollector set following find_regions_with_alloc_capacity() (rather than limiting the size of old-gen by the total capacity of the OldCollector set, which includes used memory).

Soft capacities are established, for example, by setting NewRatio or New size on the JVM command line. GenShen, for now at least, does not honor these settings. Better performance is obtained by allowing GenShen to expand and shrink generation sizes according to application behavior. This commit also tidies up various aspects of the implementation to make adjustments to generation sizing more consistent: 1. ShenandoahGlobalHeuristics::choose_global_collection_set(): share the reserves between young and old collection to maximize evacuation of garbage-first regions, regardless of whether most garbage is found in old or young 2. ShenandoahConcurrentGC::entry_final_roots(): do not balance generations before invoking finish_rebuild() because finish_rebuild will balance generations. 3. ShenandoahFreeSet::flip_to_old_gc(): invoke force_transfer_to_young() instead of transfer_to_young() so we can override soft-capacity limits 4. ShenandoahFullGC::phase5_epilog(): Do not invoke compute_balances() or balance_generations_after_rebuilding_free_set(). Allow the free-set rebuild() implementation to do this work in a more consistent fashion. 5. ShenandoahGeneration::adjust_evacuation_budgets(): replace transfer_to_youn() with force_transfer_to_young() to avoid enforcement of soft capacity limits. 6. ShenandoahGenerationSizer::force_transfer_to_young(): new method 7. ShenandoahGenerationalFullGC::balance_generations_after_gc(): establish reserves() so that free-set rebuild() can adjust balance. Do not redundantly force transfer of regions here. 8. ShenandoahGenerationalFullGC::balance_generations_after_rebuilding_free_set(): deprecate this method. 9. ShenandoahGenerationalFullGC::compute_balances(): deprecate this method. 10. ShenandoahGenerationaStatsClosure::validate_usage() (part of Shenandoah Verification): add consistency check for generation capacities

earthling-amzn · 2024-07-15T16:32:59Z

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

+  return true;
+}
+
+void ShenandoahOldHeuristics::initialize_piggyback_evacs(ShenandoahCollectionSet* collection_set,


Can we continue using mixed instead of piggyback to describe collections that include young and old regions? I think mixed is an accepted term and piggyback feels a little bit too colloquial (also not sure if the phrase is commonly known outside of English).

Good suggestion. Thanks. I'm replacing piggyback with mixed throughout (where piggyback means mixed evac). There are a few places where piggyback in Shenandoah where piggyback is used for other purposes and I'm leaving those as is.

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

…tics

ysramakrishna

Left a few suggestions; would be great to take care of the re-order thing that confuses the github diffs. Not sure if restoring old order of those methods in shenandoahOldHeuritsics.cpp is possible/easy. See comments inline.

Flushing these for now, but will continue in that file and remaining ones in a subsequent session later today/tomorrow.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.hpp

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

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

Most invocations do not need to resize generations between prepare_to_rebuild() and finish_rebuild(), so no need to make these two independent invocations.

…erves

bridgekeeper · 2024-09-10T04:22:37Z

@kdnilsen This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2024-10-08T07:03:07Z

@kdnilsen This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

Share reserves between Young Collector and Old Collector

c933d75

openjdk bot added the rfr Pull request is ready for review label Feb 12, 2024

kdnilsen marked this pull request as draft February 12, 2024 20:49

openjdk bot removed the rfr Pull request is ready for review label Feb 12, 2024

kdnilsen mentioned this pull request Feb 19, 2024

8325670: GenShen: Allow old to expand at end of each GC #394

Closed

3 tasks

kdnilsen added 13 commits February 19, 2024 17:45

Refinements and instrumentation to diagnose misbehavior

dd2a179

Allow old-gen to expand when mutator memory is available

d91bcef

Fix merge conflicts

c2cc3f6

Refine calculation of max_old_reserve

a794ca1

Reduce default value of ShenandoahOldEvacRatioPercent

6509bde

Given that mixed evacuation will sometimes borrow up to this amount of memory from the young evacuation reserve for old evacuations, we reduce the default value to reduce the impact on normal young GC.

Turn off debug instrumentation

c8e4555

Remove dead code for inelastic plabs

7bb1d38

Revert "Remove dead code for inelastic plabs"

8bc4367

This reverts commit 7bb1d38.

Round LAB sizes down rather than up to force alignment

99cce53

When we round up, we introduce the risk that the new size exceeds the maximum LAB size, resulting in an assertion error.

Revert "Round LAB sizes down rather than up to force alignment"

11b26bb

This reverts commit 99cce53.

Merge branch 'openjdk:master' into master

941d8aa

Make evacuation reserve quantities always valid

f0b15ac

In the previous implementaiton, reserve quantities were only sometimes established.

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Feb 27, 2024

kdnilsen added 2 commits March 13, 2024 13:27

Merge branch 'openjdk:master' into master

39c5885

Make satb-mode Info logging less verbose

28a382b

kdnilsen added 5 commits April 10, 2024 05:16

Merge branch 'openjdk:master' into master

a43675a

Change behavior of max_old and min_old

d881300

Revert "Change behavior of max_old and min_old"

c2cb1b7

This reverts commit d881300.

Merge branch 'openjdk:master' into master

141fec1

Merge branch 'openjdk:master' into master

bac08f0

kdnilsen added 9 commits July 8, 2024 19:54

Merge remote-tracking branch 'origin/master' into share-collector-res…

6e851cf

…erves

Fix whitespace

e69f9ac

Fix budgeting error during freeset rebuild

c2dda1b

Limit the size of old-gen by memory available in the OldCollector set following find_regions_with_alloc_capacity() (rather than limiting the size of old-gen by the total capacity of the OldCollector set, which includes used memory).

Verifier should only count non-trashed committed regions

c73e723

Turn off instrumentation

e5c1b69

Remove debug instrumentation and deprecated code

34704e4

Fix whitespace

9e14826

Remove unreferenced local variable

33eacea

kdnilsen marked this pull request as ready for review July 15, 2024 14:26

openjdk bot added the rfr Pull request is ready for review label Jul 15, 2024

earthling-amzn suggested changes Jul 15, 2024

View reviewed changes

kdnilsen added 5 commits July 16, 2024 03:31

Remove declaration of unused variables

3e38c8a

Use mixed evac rather than piggyback to describe old-gen evacuations

d05885a

Simplify arguments by using instance variables in ShenandoahOldHeuris…

406d347

…tics

Remove unreferenced variables

ee2ab01

Improve comment

21a5d32

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jul 22, 2024

ysramakrishna suggested changes Jul 25, 2024

View reviewed changes

kdnilsen added 7 commits July 26, 2024 19:20

Better comments as requested by code review

02ea566

Simplify invocations of freeset rebuild when possible

1058837

Most invocations do not need to resize generations between prepare_to_rebuild() and finish_rebuild(), so no need to make these two independent invocations.

Remove incorrect and unnecessary comments

7f01a7f

Simplify code to rebuild free set after abbreviated and old GC

e009c35

Cleanups requested by code review

699f409

Merge branch 'openjdk:master' into master

ff99de7

Merge remote-tracking branch 'origin/master' into share-collector-res…

0e555e8

…erves

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Aug 12, 2024

bridgekeeper bot closed this Oct 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8325673: GenShen: Share Reserves between Old and Young Collector #395

8325673: GenShen: Share Reserves between Old and Young Collector #395

kdnilsen commented Feb 12, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 12, 2024

mlbridge bot commented Feb 12, 2024 •

edited

Loading

openjdk bot commented Feb 27, 2024

openjdk bot commented Mar 13, 2024

earthling-amzn Jul 15, 2024

kdnilsen Jul 16, 2024

ysramakrishna left a comment

bridgekeeper bot commented Sep 10, 2024

bridgekeeper bot commented Oct 8, 2024

8325673: GenShen: Share Reserves between Old and Young Collector #395

8325673: GenShen: Share Reserves between Old and Young Collector #395

Conversation

kdnilsen commented Feb 12, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewing

Webrev

bridgekeeper bot commented Feb 12, 2024

mlbridge bot commented Feb 12, 2024 • edited Loading

Webrevs

openjdk bot commented Feb 27, 2024

openjdk bot commented Mar 13, 2024

earthling-amzn Jul 15, 2024

Choose a reason for hiding this comment

kdnilsen Jul 16, 2024

Choose a reason for hiding this comment

ysramakrishna left a comment

Choose a reason for hiding this comment

bridgekeeper bot commented Sep 10, 2024

bridgekeeper bot commented Oct 8, 2024

kdnilsen commented Feb 12, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 12, 2024 •

edited

Loading