RFR: 8319373: Serial: Refactor dirty cards scanning during Young GC [v10]

[Kim Barrett]

On Mon, 20 Nov 2023 15:19:58 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Reading the new code directly is probably easier. The structure is quite similar to its counterpart in Parallel.
>> 
>> It's mostly perf-neutral, except when dirty cards are scarce. Using `card_scan.java` in [JDK-8310031](https://bugs.openjdk.org/browse/JDK-8310031), I observed ~40% reduction in young-gc pause time with `stride = 32 * 64`.
>
> Albert Mingkun Yang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   review

A few more minor nits.

src/hotspot/share/gc/serial/cardTableRS.cpp line 351:

> 349: CardTable::CardValue* CardTableRS::find_first_dirty_card(CardValue* const start_card,
> 350:                                                          CardValue* const end_card) {
> 351:   using Word = uintptr_t;

I think it would be better if Word was consistent with clean_card_row_val, to eliminate casts now and in
the future when you address my earlier comment about needing a clean-row accessor.  So I suggest
`using Word = decltype(clean_card_row_val());`.
Maybe as part of the clean-row accessor also add a CardRow type alias.
Note that I think that type should be unsigned (as currently here, e.g. uintptr_t) rather than signed
(as currently provided by clean_card_row_val).

src/hotspot/share/gc/serial/cardTableRS.cpp line 353:

> 351:   using Word = uintptr_t;
> 352: 
> 353:   static_assert(clean_card_val() == (CardValue)-1, "inv");

Now that clean_card_row_val() is being used, why do we need this static_assert?

src/hotspot/share/gc/serial/cardTableRS.cpp line 354:

> 352: 
> 353:   static_assert(clean_card_val() == (CardValue)-1, "inv");
> 354:   constexpr Word clean_word = (Word)CardTable::clean_card_row_val();

Why is clean_card_row_val qualified here?

src/hotspot/share/gc/serial/cardTableRS.cpp line 356:

> 354:   constexpr Word clean_word = (Word)CardTable::clean_card_row_val();
> 355: 
> 356:   CardValue* i_card = start_card;

What does the "i_" prefix here mean?  Something like "cur_card" might be a better name.

src/hotspot/share/gc/serial/cardTableRS.cpp line 371:

> 369:   while (i_card + sizeof(Word) <= end_card) {
> 370:     Word* i_word = reinterpret_cast<Word*>(i_card);
> 371:     if (*i_word != clean_word) {

Why not just use clean_card_row_val() here directly? Rather than introducing the clean_word variable and
only using it once, here.

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

Changes requested by kbarrett (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16492#pullrequestreview-1740535865
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1399651020
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1399642167
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1399651910
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1399634692
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1399646352