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

Fri Nov 10 10:19:10 UTC 2023

On Mon, 6 Nov 2023 12:01:32 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 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:
> 
>  - review
>  - Merge branch 'master' into s1-young-gc
>  - s1-young-gc

Changes requested by tschatzl (Reviewer).

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

> 351: 
> 352:   auto is_word_aligned = [] (const void* const p) {
> 353:       return is_aligned(p, sizeof(Word));

The include for `is_aligned` seems to be missing.

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

> 370: 
> 371:   // Word comparison
> 372:   while (i_card + sizeof(Word) <= end_card) {

Not sure if it is worth mentioning, but this should probably increment by `sizeof(Word) / sizeof(CardValue)`.

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

> 373:     Word* i_word = reinterpret_cast<Word*>(i_card);
> 374:     if (*i_word != clean_word) {
> 375:       // Found sth in this word; fall back to byte-comparison

Suggestion:

      // Found a clean card in this word; fall back to per-CardValue comparison.

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

> 376:       break;
> 377:     }
> 378:     i_card += sizeof(Word);

`sizeof(Word) / sizeof(CardValue)` ? Or assert that `sizeof(CardValue) == 1` somewhere.

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

> 379:   }
> 380: 
> 381:   // Byte comparison

// Per-CardValue comparison.

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

> 400:     }
> 401: 
> 402:     // a potential candidate

Suggestion:

    // A potential candidate.

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

> 408:     }
> 409: 
> 410:     // final obj in dirty-chunk crosses card-boundary

Suggestion:

    // Last object in dirty chunk crosses card-boundary.

I think "last" is the more appropriate term compared to "final" as to me "final" indicates that this is the overall very last object.

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

> 411:     oop obj = cast_to_oop(obj_start_addr);
> 412:     if (obj->is_objArray()) {
> 413:       // precised-marked

Suggestion:

      // ObjArrays are always precisely-marked so we are not allowed to jump to the end of the current object.

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

> 415:     }
> 416: 
> 417:     // final card occupied by this obj

Suggestion:

    // Final card occupied by this obj. We can use this to return and continue our search because this object is imprecisely marked and is always looked at for references as a whole anyway.

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

> 421:     }
> 422: 
> 423:     // continue the search...

Suggestion:

    // Continue the search at the end of the current object...

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

> 456:     HeapWord* start_addr;
> 457:     HeapWord* end_addr;
> 458:   } cached_obj {nullptr, mr.start()};

Suggestion:

  } cached_obj { nullptr, mr.start() };

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

> 475: 
> 476:   CardValue* const clear_limit_card = is_card_aligned(mr.end()) ? end_card - 1
> 477:                                                                 : end_card - 2;

Please add a comment similar to the one in the `PSStripeShadowCardTable` constructor here, otherwise this code will be too hard to understand in six months.

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

> 481: 
> 482:     if (dirty_l == end_card) {
> 483:       // no dirty cards

Suggestion:

      // No dirty cards to iterate.

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

> 507: 
> 508:       if (is_obj_array) {
> 509:         // precise-marked

Suggestion:

        // ObjArrays are always precise-marked.

src/hotspot/share/gc/serial/cardTableRS.hpp line 59:

> 57:                                               CardValue* const end_card);
> 58:   template<typename Func>
> 59:   CardTable::CardValue* find_first_clean_card(CardValue* const start_card,

Already mentioned that this method should probably be renamed or at least documented more exactly what it does with all its optimizations.

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

PR Review: https://git.openjdk.org/jdk/pull/16492#pullrequestreview-1724331448
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1389115818
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1389127249
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1389125502
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1389127985
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1389126089
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1389112450
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1389112730
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1389134383
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1389113011
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1389113241
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1389136146
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1389142118
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1389169318
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1389172947
PR Review Comment: https://git.openjdk.org/jdk/pull/16492#discussion_r1389179817