8214237: Join parallel phases post evacuation #3811

tschatzl · 2021-04-30T08:21:35Z

Hi all,

can I get reviews for this change that joins all non-object moving post evacuation tasks into two parallel batched phases?

This has the advantage of avoiding spinup and teardown of the separate tasks and also makes a few tasks that are done serially at the moment run in parallel to each other.

For that, extensive analysis of dependencies has been performed, and I found that we need two G1BatchedGangTasks (introduced just for this purpose):

The first one, G1PostEvacuateCollectionSetCleanupTask1 contains the following subtasks:

Merge PSS (serial)
Recalculate Used (serial)
Remove Self Forwards (parallel, on evacuation failure)
Clear Card Table (parallel)

The second one, G1PostEvacuateCollectionSetCleanupTask2 contains the following subtasks:

Eagerly Reclaim Humongous Objects (serial)
Purge Code Roots (serial)
Reset Hot Card Cache (serial)
Update Derived Pointers (serial)
Redirty Logged Cards (parallel)
Restore Preserved Marks (parallel, on evacuation failure)
Free Collection Set (parallel)

Feel free to propose better names for the batch tasks :) The distribution has been somewhat arbitrary with the following limitations:

Redirty Logged Cards depends on Clear Card Table - we need to clear card table scanning information from the card table before redirtying (Clear Card Table could actually split into the part that clears the "Young Gen" marks and the parts that actually contain card scan information, but it does not seem to provide an advantage).
Redirty Cards depends on *Merge PSS - that phase merge per thread DCQs into global dcq; redirty cards could of course take from the per-thread dcq if needed
Free Collection Set depends on Merge PSS to merge per thread surviving young words to single one and Recalculate Used as it updates the heap usage counters.

More dependencies between other phases not merged are noted on the JDK-8214237 page.

This change seems huge (~1k LOC changes), but actually it isn't: lots of code has been moved 1:1 from g1CollectedHeap.cpp to the new g1YoungGCPostEvacuateTasks.* files. g1CollectedHeap.cpp just got way too huge with this change so it was really useful to start splitting the young gc related things into new files like has been done for the full GC. I'll continue extending this move, but there just has to be a start somewhere.

I tried to do this change in a few steps, but that unfortunately caused too many weird intermediate workarounds steps in the timing code (G1GCPhaseTimes). So let me try to cut down the change into steps for you.

So the change can be split up into

move functionality to g1YoungGCPostEvacuateTasks.cpp: this affected dismantling existing AbstractGangTasks into main code (constructor, destructor, do_work method) and move that code to the corresponding G1AbstractSubTask.
The helper code typically inlined into that AbstractGangTask were moved as well close to that G1AbstractSubTask.
wiring up stuff to work with G1BatchedGangTask

And the review tasks should roughly be:

verify that the copy&paste has been correct (there is a list below)
verify that dependencies are correct (see above)
minor wiring changes

Here's a summary for the copy&paste changes:

RedirtyLoggedCardTableEntryClosure moved verbatim to g1YoungGCPostEvacuateTasks.cpp
G1RedirtyLoggedCardsTask were transformed into a G1AbstractSubTask
G1CollectedHeap::remove_self_forwarding_pointers() and G1CollectedHeap::restore_after_evac_failure were transformed into a G1AbstractSubTask
recalculate used memory activity has been extracted from the code and put into G1PostEvacuateCollectionSetCleanupTask1::RecalculateUsedTask
serial tasks that were about calling a single method were wrapped into a G1AbstractSubTask that just calls the same method
G1FreeCollectionSetTask moved to G1PostEvacuateCollectionSetCleanupTask2::FreeCollectionSetTask
G1FreeHumongousRegionClosure moved verbatim to g1YoungGCPostEvacuateTasks.cpp, and wrapped into a G1AbstractSubTask

Minor other things:

the condition for executing and printing logs for when doing eager reclaim has been unified: sometimes the logs have been printed without any eager reclaim because they did not correspond

Feel free to ask questions.

Testing: tier1-5 multiple times

Thanks,
Thomas

Progress

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

Issue

JDK-8214237: Join parallel phases post evacuation

Reviewers

Ivan Walulya (@walulyai - Committer) ⚠️ Review applies to a0fe408
Stefan Johansson (@kstefanj - Reviewer) ⚠️ Review applies to a0fe408

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3811/head:pull/3811
$ git checkout pull/3811

Update a local copy of the PR:
$ git checkout pull/3811
$ git pull https://git.openjdk.java.net/jdk pull/3811/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3811

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3811.diff

bridgekeeper · 2021-04-30T08:28:14Z

👋 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 · 2021-04-30T08:29:56Z

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

hotspot

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.

mlbridge · 2021-04-30T09:02:14Z

Webrevs

tschatzl · 2021-04-30T09:35:22Z

/label add hotspot-gc
/label remove hotspot

openjdk · 2021-04-30T09:35:42Z

@tschatzl
The hotspot-gc label was successfully added.

openjdk · 2021-04-30T09:36:41Z

@tschatzl
The hotspot label was successfully removed.

walulyai · 2021-05-03T10:49:35Z

src/hotspot/share/gc/g1/g1CollectedHeap.hpp

-  // Stores whether during humongous object registration we found candidate regions.
-  // If not, we can skip a few steps.
-  bool _has_humongous_reclaim_candidates;
+  uint _num_humongous_reclaim_total; // Total number of humongous objects


Comment might be misleading

tschatzl · 2021-05-03T13:20:46Z

The change adds:

constant for "almost no work" to do for a sub task to avoid random inconsistent assignment without reason
renamed the sub tasks execute methods to should_execute to better indicate that these are predicates
tried to improve the mentioned comment
as per discussion with Ivan

walulyai

lgtm!

kstefanj

This looks good, have a few small suggestions on names but nothing major. I did not check every copy-paste in detail, but skimmed it.

I suspect getting some strange numbers in the logging might be one risk, have you done any manual runs trying to compare before and after making sure everything looks reasonable.

kstefanj · 2021-05-06T11:08:12Z

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

+bool G1CollectedHeap::eagerly_reclaim_enabled() const {
+  return (G1EagerReclaimHumongousObjects &&
+          (G1CollectedHeap::heap()->has_humongous_reclaim_candidate_objects() || log_is_enabled(Debug, gc, humongous)));


This is not really "eager reclaim enabled" but "should do eager reclaim". It also feels a bit strange to have the logging check here. I see why it is needed but I wonder if it should be broken out to be two different function. Something like has_eager_reclaim_work() and do_eager_reclaim_logging().

src/hotspot/share/gc/g1/g1CollectedHeap.hpp

kstefanj · 2021-05-06T11:30:01Z

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

-  _preserved_marks_set.assert_empty();
-
-  merge_per_thread_state_info(per_thread_states);
+  post_evacuate_cleanup_1(per_thread_states, rdcqs);


Would it make sense to include a short comment about what these two cleanup steps do?

I fear that is going to be stale fairly quickly - the batchedgangtasks already contain a description of all tasks run within them, repeating that here seems just a way of waiting for it to get stale. I can't find a useful generic description either, so I'll leave it as is for now.

src/hotspot/share/gc/g1/g1GCPhaseTimes.hpp

src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.hpp

openjdk · 2021-05-06T13:36:58Z

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

8214237: Join parallel phases post evacuation

Reviewed-by: iwalulya, sjohanss

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

ebb68d2: 8049700: Deprecate obsolete classes and methods in javax/swing/plaf/basic
3a474d9: 8265612: revise the help info for jmap histo command
c97f56c: 8266172: -Wstringop-overflow happens in vmError.cpp
43ad24f: 8265465: jcmd VM.cds should keep already dumped archive when exception happens
66191ff: 8266193: BasicJMapTest does not include testHistoParallel methods
36e5ad6: 8263236: runtime/os/TestTracePageSizes.java fails on old kernels
0ca86da: 8266002: vmTestbase/nsk/jvmti/ClassPrepare/classprep001 should skip events for unexpected classes
52f1db6: 8262002: java/lang/instrument/VerifyLocalVariableTableOnRetransformTest.sh failed with "TestCaseScaffoldException: DummyClassWithLVT did not match .class file"
04f7112: 8266293: Key protection using PBEWithMD5AndDES fails with "java.security.InvalidAlgorithmParameterException: Salt must be 8 bytes long"
a90b33a: 8266573: Make sure blackholes are tagged for all JVMCI paths
... and 89 more: https://git.openjdk.java.net/jdk/compare/51b218842f001f1c4fd5ca7a02a2ba21e9e8a82c...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.

tschatzl · 2021-05-07T09:32:26Z

Thanks @kstefanj @walulyai for your reviews.

tschatzl · 2021-05-07T11:37:00Z

Github actions completed fine with the latest change. Integrating.

/integrate

openjdk · 2021-05-07T11:38:52Z

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

2798b0d: 8266349: Pass down requested page size to reserve_memory_special
e0c8688: 8262992: Improve @see output
d2b5350: 8263507: Improve structure of package summary pages
a65021e: 8266618: Remove broken -XX:-OptoRemoveUseless
94c6177: 8266536: Provide a variant of os::iso8601_time which works with arbitrary timestamps
71b8ad4: 8266609: AArch64: include FP/LR space in LIR_Assembler::initial_frame_size_in_bytes()
ebb68d2: 8049700: Deprecate obsolete classes and methods in javax/swing/plaf/basic
3a474d9: 8265612: revise the help info for jmap histo command
c97f56c: 8266172: -Wstringop-overflow happens in vmError.cpp
43ad24f: 8265465: jcmd VM.cds should keep already dumped archive when exception happens
... and 95 more: https://git.openjdk.java.net/jdk/compare/51b218842f001f1c4fd5ca7a02a2ba21e9e8a82c...master

Your commit was automatically rebased without conflicts.

Pushed as commit 14f0afe.

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

Initial version

167a444

openjdk bot added the hotspot hotspot-dev@openjdk.org label Apr 30, 2021

Whitespace fixes

1be3f1c

openjdk bot added the rfr Pull request is ready for review label Apr 30, 2021

openjdk bot added hotspot-gc hotspot-gc-dev@openjdk.org and removed hotspot hotspot-dev@openjdk.org labels Apr 30, 2021

Remove test debug code

5526184

walulyai reviewed May 3, 2021

View reviewed changes

iwalulya review

009afcd

iwalulya: more renaming

a0fe408

walulyai approved these changes May 3, 2021

View reviewed changes

kstefanj approved these changes May 6, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 6, 2021

sjohanss review

2a75865

openjdk bot closed this May 7, 2021

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 May 7, 2021

mgkwill mentioned this pull request May 7, 2021

8256155: Allow multiple large page sizes to be used on Linux #1153

Closed

3 tasks

tschatzl deleted the submit/8214237-join-post-evacuate2 branch May 11, 2021 11:48

8214237: Join parallel phases post evacuation #3811

8214237: Join parallel phases post evacuation #3811

Uh oh!

Conversation

tschatzl commented Apr 30, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Apr 30, 2021

Uh oh!

openjdk bot commented Apr 30, 2021

Uh oh!

mlbridge bot commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

tschatzl commented Apr 30, 2021

Uh oh!

openjdk bot commented Apr 30, 2021

Uh oh!

openjdk bot commented Apr 30, 2021

Uh oh!

walulyai May 3, 2021

Choose a reason for hiding this comment

Uh oh!

tschatzl commented May 3, 2021

Uh oh!

walulyai left a comment

Choose a reason for hiding this comment

Uh oh!

kstefanj left a comment

Choose a reason for hiding this comment

Uh oh!

kstefanj May 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kstefanj May 6, 2021

Choose a reason for hiding this comment

Uh oh!

tschatzl May 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openjdk bot commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tschatzl commented May 7, 2021

Uh oh!

tschatzl commented May 7, 2021

Uh oh!

openjdk bot commented May 7, 2021

Uh oh!

Uh oh!

tschatzl commented Apr 30, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 30, 2021 •

edited

Loading

openjdk bot commented May 6, 2021 •

edited

Loading