[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    Re: RFR: 8214237: Join parallel phases post evacuation [v2]
From:       Thomas Schatzl <tschatzl () openjdk ! java ! net>
Date:       2021-04-30 9:43:29
Message-ID: iAbrv1TDPDgaC6MyclIlbHftdtPyuIqEl10Dq6_n--A=.7c7b5532-b214-4acf-a211-c52d3f1a2f9f () github ! com
[Download RAW message or body]

> 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

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3811/files
  - new: https://git.openjdk.java.net/jdk/pull/3811/files/1be3f1c9..5526184c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3811&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3811&range=00-01

  Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3811.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3811/head:pull/3811

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


[prev in list] [next in list] [prev in thread] [next in thread]