RFR: 8305062: Refactor CardTable::resize_covered_region

[Thomas Schatzl]

On Tue, 28 Mar 2023 09:59:51 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

The change is okay.

In some way I would have preferred a larger refactoring:
* keeping both `_covered` and `_committed` seems unnecessary; the latter can always be calculated from the former (easily, with negligible overhead since we are committing there), so it seems to be somewhat of a waste of space. 
* `_committed` seems to be only ever actually used in `CardTable::resize_covered_region` too.
* `CardTable::resize_covered_region` requires that the given region start at either of the two covered regions, but still allows passing an arbitrary region. Maybe some more specialization just for a serial/parallel gc card table would be nice as it's only used by them. In that sense, the `initialize()` method also seems to be specific to serial/parallel.

None of that necessary in context of this particular change, it does improve the code. Maybe something to record in the bug tracker.

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

> 75:   _byte_map_base(nullptr),
> 76:   _covered(MemRegion::create_array(_max_covered_regions, mtGC)),
> 77:   _committed(MemRegion::create_array(_max_covered_regions, mtGC)),

Is it useful any more to dynamically create the `_committed` and `_covered` `MemRegions`? These are fixed size anyway. Also the code unconditionally allocates them even if not both are used. It seems to be a win to just statically allocate them.

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

> 127:   log_trace(gc, barrier)("CardTable::CardTable: ");
> 128:   log_trace(gc, barrier)("    &_byte_map[0]: " PTR_FORMAT "  &_byte_map[last_valid_index()]: " PTR_FORMAT,
> 129:                   p2i(&_byte_map[0]), p2i(&_byte_map[last_valid_index()]));

pre-existing: indentation of arguments

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

> 151:   assert(UseSerialGC || UseParallelGC, "only these two collectors");
> 152:   assert(_whole_heap.contains(new_region),
> 153:            "attempt to cover area not in reserved area");

pre-existing: indentation

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

> 157:   assert(_committed[idx].start() != nullptr, "precondition");
> 158: 
> 159:   // We don't change the start of a region, only the end.

Suggestion:

  // We don't allow changes to the start of a region, only the end.

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

> 167: 
> 168:   if (idx == 0) {
> 169:     // in case the card for gen-boundary is not page-size aligned

Suggestion:

    // In case the card for gen-boundary is not page-size aligned, make sure to not uncommit it erroneously.

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

> 178: 
> 179:   if (new_committed.word_size() > _committed[idx].word_size()) {
> 180:     // expand

Suggestion:

    // Expand.

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

> 190:     memset(delta.start(), clean_card, delta.byte_size());
> 191:   } else {
> 192:     // shrink

Suggestion:

    // Shrink.

src/hotspot/share/gc/shared/cardTable.hpp line 57:

> 55:   // "covering" parts of the heap that are committed. At most one covered
> 56:   // region per generation is needed.
> 57:   static constexpr int _max_covered_regions = 2;

Suggestion:

  static constexpr int max_covered_regions = 2;

Naming of this static const should follow existing practice in this file (at least ;) )

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

Changes requested by tschatzl (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13206#pullrequestreview-1387863741
PR Review Comment: https://git.openjdk.org/jdk/pull/13206#discussion_r1168593017
PR Review Comment: https://git.openjdk.org/jdk/pull/13206#discussion_r1168585816
PR Review Comment: https://git.openjdk.org/jdk/pull/13206#discussion_r1168568594
PR Review Comment: https://git.openjdk.org/jdk/pull/13206#discussion_r1168611144
PR Review Comment: https://git.openjdk.org/jdk/pull/13206#discussion_r1168588167
PR Review Comment: https://git.openjdk.org/jdk/pull/13206#discussion_r1168589044
PR Review Comment: https://git.openjdk.org/jdk/pull/13206#discussion_r1168589233
PR Review Comment: https://git.openjdk.org/jdk/pull/13206#discussion_r1168703736