RFR: 8252752: Clear card table for old regions during scan in G1

[Ivan Walulya]

On Thu, 24 Sep 2020 19:12:35 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

> Hi all,
> 
>   can I get reviews for this change that removes the need for explicit clearing of the card table for typically a large
>   amount of regions in most cases?
> 
> Currently g1 unconditionally marks scanned dirty cards as "scanned" to remember that they had already been scanned in
> earlier evacuation passes. Then in the "clear card table phase" g1 clears all these marks including the card table of
> evacuated regions.  This change modifies g1 so that if possible, it clears dirty cards after scanning them instead of
> setting them to "scanned" if there is only one evacuation pass. This saves a lot of work later (e.g. the amount of
> regions to clear in the clear card table phase may be reduced by a magnitude or more, depending on the ration between
> evacuated and scanned regions).  The main change here is which type of regions (ones in the collection set, ones that
> are only scanned) are put into which list of regions g1 already manages: there is one "all" list containing all regions
> that need clearing, and a "next" list containing the regions to be scanned in the current evacuation pass. Previously
> g1 put regions into at least one list; now regions in the current collection set are always directly put into the "all"
> list (as they independently of other reasons require card table cleaning), while to-be-scanned regions are always only
> put into the "next" list.  Then, to achieve the desired effect that only regions that absolutely require clearing are
> in the "all" list, g1 does not merge the "next" list into the "all" list when it detects that there will only be one
> evacuation pass.  Performance impact: Decreases clear card table time significantly if the number of regions in the
> collection set is much smaller than the number of scanned regions (as it decreases the number of regions to clear).
> Some cursory perf testing showed slight overall pause time improvements.  Testing: hs-tier1-5 ("Initial import" change)
> hs-tier1-2 (latest)

minor comments

src/hotspot/share/gc/g1/g1CardTable.hpp line 107:

> 105:   inline size_t mark_region_dirty(size_t start_card_index, size_t num_cards);
> 106:
> 107:   // Mark the given range of cards to "which". All of these cards must be Dirty.

Maybe change the comment to "Mark the given range of dirty cards as..." or the function name to "mark_dirty_to"

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

> 3954:     }
> 3955:
> 3956:     rem_set()->complete_evac_phase(true /* has_more_than_one_evacuation_phase */);

I think it would be cleaner with a local variable:
bool has_more_than_one_evacuation_phase = true;
rem_set()->complete_evac_phase(has_more_than_one_evacuation_phase);

Instead of the comment, same with remember_already_scanned_cards comment above

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

Marked as reviewed by iwalulya (Author).

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