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

Thu May 6 13:39:13 UTC 2021

On Mon, 3 May 2021 13:32:24 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:
> 
>   iwalulya: more renaming

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.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 3422:

> 3420: bool G1CollectedHeap::eagerly_reclaim_enabled() const {
> 3421:   return (G1EagerReclaimHumongousObjects &&
> 3422:           (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.cpp line 3852:

> 3850:   _allocator->release_gc_alloc_regions(evacuation_info);
> 3851: 
> 3852:   post_evacuate_cleanup_1(per_thread_states, rdcqs);

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

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

> 259:   HumongousReclaimCandidates _humongous_reclaim_candidates;
> 260:   uint _num_humongous_reclaim_total_objects; // Current amount of (all) humongous objects found in the heap.
> 261:   uint _num_humongous_reclaim_candidate_objects; // Number of humongous object eager reclaim candidates.

I see this has already been change i bit, but I think `_num_humongous_reclaim_total_objects` could drop both the reclaim and total part and just be `_num_humongous_objects` if this is really what it is. And maybe also shorten the other one by removing objects and call it `_num_humongous_reclaim_candidates`, but I don't feel super strong about the other one. The getters below should also match the members I think.

src/hotspot/share/gc/g1/g1GCPhaseTimes.hpp line 163:

> 161: 
> 162:   double _cur_post_evacuate_cleanup_1_time_ms;
> 163:   double _cur_post_evacuate_cleanup_2_time_ms;

`double _cur_clear_ct_time_ms;` below is now unused. Same for `_recorded_merge_pss_time_ms` but that one seems to have been unused for a while. Please remove these two.

src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.hpp line 72:

> 70: 
> 71: class G1PostEvacuateCollectionSetCleanupTask1::RemoveSelfForwardPtrsTask : public G1AbstractSubTask {
> 72:   G1ParRemoveSelfForwardPtrsTask _cl;

Nit picking but I would prefer _task.

src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.hpp line 153:

> 151: class G1PostEvacuateCollectionSetCleanupTask2::RestorePreservedMarksTask : public G1AbstractSubTask {
> 152:   PreservedMarksSet* _preserved_marks;
> 153:   AbstractGangTask* _cl;

Again, I think `_task` makes more sense. Especially since it is initialized with `create_task()`.

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

Marked as reviewed by sjohanss (Reviewer).

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