RFR: 8305062: Refactor CardTable::resize_covered_region [v2]

[Thomas Schatzl]

On Tue, 18 Apr 2023 08:33:53 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Simple refactoring to make logic around cardtable cover-region more concrete, since #generations and gen-boundary is fixed for Serial/Parallel.
>> 
>> Test: tier1-6
>
> Albert Mingkun Yang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - remove-committed
>  - review
>  - card-table-clear-memregion

Lgtm. Removal of `_committed` improved the code significantly imho.

Please consider the other formatting suggestions.

src/hotspot/share/gc/shared/cardTable.cpp line 136:

> 134:   HeapWord* addr_r = mr.is_empty()
> 135:                        ? addr_l
> 136:                        : (HeapWord*)align_up(byte_after(mr.last()), _page_size);

I think common formattings for multiline ?: statements are:


  HeapWord* addr_r = mr.is_empty() ?
                     addr_l :
                     (HeapWord*)align_up(byte_after(mr.last()), _page_size);

or


  HeapWord* addr_r = mr.is_empty()
                   ? addr_l
                   : (HeapWord*)align_up(byte_after(mr.last()), _page_size);

or in this case:

  HeapWord* addr_r = mr.is_empty() ? addr_l : (HeapWord*)align_up(byte_after(mr.last()), _page_size);


At least the "?"/":" in front of the line, indented 4 characters to anything above is very unusual.

I did see this style (4 chars indented) once in gc code though. I would like to avoid further proliferation of this one; for that particular case it's imo best to just use a single line here.

src/hotspot/share/gc/shared/cardTable.cpp line 139:

> 137: 
> 138:   if (mr.start() == _covered[0].start()) {
> 139:     // In case the card for gen-boundary is not page-size aligned, the crossing page belongs to _covered[1]

Suggestion:

    // In case the card for gen-boundary is not page-size aligned, the crossing page belongs to _covered[1].

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

Marked as reviewed by tschatzl (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13206#pullrequestreview-1389605436
PR Review Comment: https://git.openjdk.org/jdk/pull/13206#discussion_r1169703067
PR Review Comment: https://git.openjdk.org/jdk/pull/13206#discussion_r1169704988