RFR: 8214237: Join parallel phases post evacuation [v2]

[Ivan Walulya]

On Fri, 30 Apr 2021 09:43:29 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> 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](https://bugs.openjdk.java.net/browse/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
>
> Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove test debug code

src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 260:

> 258: 
> 259:   HumongousReclaimCandidates _humongous_reclaim_candidates;
> 260:   uint _num_humongous_reclaim_total; // Total number of humongous objects

Comment might be misleading

src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 263:

> 261:   uint _num_humongous_reclaim_candidates; // Total number of humongous object eager reclaim candidates
> 262: public:
> 263:   uint num_humongous_objects_total() const { return _num_humongous_reclaim_total; }

now the method name is misleading

-------------

PR: https://git.openjdk.java.net/jdk/pull/3811