RFR: 8275056: Virtualize G1CardSet containers over heap region [v8]

[Albert Mingkun Yang]

On Mon, 8 Nov 2021 15:11:53 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Hi all,
>> 
>> can I have reviews for this change that virtualizes `G1CardSet` "regions" over a heap region, allowing the use of multiple "`G1CardSet` card regions" across a single heap region?
>> 
>> I.e. `HeapRegionRemSet`, which is the interface to a region's card set, simply uses multiple indexes for the remembered set of a single source heap region if necessary. E.g. on a 128MB region, heap region 0's cards would be stored as (what I call) "card region" indexes 0..3 as appropriate in its `_card_set`.
>> 
>> When retrieving the values, the appropriate retransformation is done (during `HeapRegionRemSet::iterate_for_merge()`).
>> 
>> Assigning `HeapRegionRemSet` to handle all this multiplexing required some move of the `G1CardSet::iterate_for_merge` method to `HeapRegionRemSet`, which is why there are more changes than expected.
>> 
>> One change I would like to have opinions on is storing the amount of card regions per region into `G1CardSetConfiguration`, maybe it is better to put this into `HeapRegionRemSet` - but I did not want to start a `HeapRegionRemSetConfiguration` (maybe also put the cached values introduced in the `split_card` optimization there as well?).
>> 
>> This allows unlimited actual heap region size. Currently set to 512MB (what we would set ergonomically if on a 1 TB heap), but that's just a random number basically.
>> Feel free to suggest a different maximum heap region size if any. We could also keep the ergonomics use a smaller heap region size (e.g. 32M as before).
>> 
>> There is also a CSR to look at.
>> 
>> Testing: tier1-5, some perf testing on region sizes up to 512M with slight improvements in specjbb2015 with larger region sizes.
>> 
>> Thanks,
>> Thomas
>
> Thomas Schatzl has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits:
> 
>  - Merge branch 'master' into 8275056-virtualize-g1cardset-containers2
>  - ayang review1
>  - Limit ergonomics to 32m regions
>  - sjohanss review
>  - Fix assert after merge - the assert depends on current heap region configuration, so move it to the correct constructor
>  - Merge branch 'master' into 8275056-virtualize-g1cardset-containers2
>  - 32 bit compatibility - limit region size to 32M there as before. Fix test.
>  - Virtual heap regions in remembered sets

Just some minor comments/suggestions.

src/hotspot/share/gc/g1/g1CardSet.cpp line 67:

> 65:                          default_log2_card_region_per_region())                     /* log2_card_region_per_region */
> 66: {
> 67:   assert((_log2_card_region_per_heap_region + _log2_card_region_size) == (uint)HeapRegion::LogCardsPerRegion,

I suggest `_log2_card_region_size -> _log2_cards_per_card_region`; then the assertion becomes more consistent, IMO.
`assert((_log2_card_region_per_heap_region + _log2_cards_per_card_region) == (uint)HeapRegion::LogCardsPerRegion)`, which, in math form, is
`(card_region / heap_region) x (cards / card_region) == cards / (heap_)region`

src/hotspot/share/gc/g1/heapRegionRemSet.inline.hpp line 105:

> 103:                              region_idx >> _log_card_regions_per_region,
> 104:                              (region_idx & _card_regions_per_region_mask) << _log_card_region_size);
> 105:     _card_set->iterate_cards_or_ranges_in_container(card_set, cl);

`region_idx` is actually `card_region_idx`, right? If so, `card_region_idx >> _log_card_regions_per_region` ==> `region_idx` (the second arg of `G1ContainerCardsOrRanges` constructor) makes sense to me.

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

Marked as reviewed by ayang (Reviewer).

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