RFR: 8305060: G1: Refactor G1ScanHRForRegionClosure::scan_heap_roots

[Thomas Schatzl]

On Tue, 28 Mar 2023 09:48:34 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

> Simple refactoring to separate out dirty-cards iteration logic from dirty-cards processing.
> 
> Test: tier1-6

Lgtm sans improved naming.

src/hotspot/share/gc/g1/g1RemSet.cpp line 575:

> 573:   }
> 574: 
> 575:   class ClaimedChunk {

Please add a short description of the class. `ClaimedChunk` isn't really too descriptive or at least not related it's used.
With that name I would expect a simple data class holding information about the chunk, not necessarily iterating the (dirty) blocks over a chunk. I.e. I believe that the original name `CardTableScanner` was better, but in this case maybe something like `ChunkScanner` (random name).

src/hotspot/share/gc/g1/g1RemSet.cpp line 644:

> 642:     }
> 643:   public:
> 644:     ClaimedChunk(CardValue* const start_card, CardValue* const end_card) :

There should be a newline here.

src/hotspot/share/gc/g1/g1RemSet.cpp line 697:

> 695:                              size_t num_cards = dirty_r - dirty_l;
> 696:                              do_claimed_block(region_idx, dirty_l, num_cards);
> 697:                              _blocks_scanned++;

We already talked about this privately, most, if not all of this code may be better located in `do_claimed_block`. Not sure if it isn't small enough of a change to do this here already.

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

Marked as reviewed by tschatzl (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13205#pullrequestreview-1367018236
PR Review Comment: https://git.openjdk.org/jdk/pull/13205#discussion_r1154526008
PR Review Comment: https://git.openjdk.org/jdk/pull/13205#discussion_r1154539399
PR Review Comment: https://git.openjdk.org/jdk/pull/13205#discussion_r1154542176