RFR: 8325725: Parallel: Refactor PSParallelCompact::fill_dense_prefix_end [v2]

[Thomas Schatzl]

On Mon, 26 Feb 2024 15:39:17 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Refactor the logic around creating filler obj before prefix-end.
>> 
>> 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 one additional commit since the last revision:
> 
>   pgc-end-bit

Changes requested by tschatzl (Reviewer).

src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1448:

> 1446:   HeapWord* const dense_prefix_end = dense_prefix(id);
> 1447:   RegionData* const region = _summary_data.addr_to_region_ptr(dense_prefix_end);
> 1448:   idx_t const dense_prefix_bit = _mark_bitmap.addr_to_bit(dense_prefix_end);

Could probably be moved into the scope using it.

src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1450:

> 1448:   idx_t const dense_prefix_bit = _mark_bitmap.addr_to_bit(dense_prefix_end);
> 1449: 
> 1450:   if (region->partial_obj_size() != 0 ||

Maybe document the first condition; it's probably something like "An object spans beyond the dense prefix, so the previous region is completely filled up and there is no need to add a filler object".
Alternatively make the condition more clear by renaming `region` to `region_after_dense_prefix` or something.
It just took long enough for me to understand what this means to warrant some better documentation. In hindsight maybe this is just because my knowledge of Parallel GC is limited and it took quite a while to understand various invariants of this code :)

src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1452:

> 1450:   if (region->partial_obj_size() != 0 ||
> 1451:       _mark_bitmap.is_obj_beg(dense_prefix_bit)) {
> 1452:     // next region starts with live bytes

Suggestion:

    // The region after the dense prefix starts with live bytes.

or
Suggestion:

    // The region after the dense prefix starts with live bytes. This can either be an object crossing into that region, or an object starting in that region. The latter implies that any dead space before must at least be of CollectedHeap::min_fill_size() size because it must have been a regular allocation.

to cover this and the comment above. It's a bit wordy though.

src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1456:

> 1454:   }
> 1455: 
> 1456:   if (_mark_bitmap.is_obj_end(dense_prefix_bit - 2)) {

I would almost recommend inverting this condition with a return, but feel free to ignore.

src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1458:

> 1456:   if (_mark_bitmap.is_obj_end(dense_prefix_bit - 2)) {
> 1457:     // Exactly one heap word gap right before dense prefix end; the filler obj
> 1458:     // will extend to next region.

Suggestion:

    // There is exactly one heap word gap right before the dense prefix end, so we need a filler object.
    // It extends into the region after the last dense prefix region.

src/hotspot/share/gc/parallel/psParallelCompact.cpp line 1459:

> 1457:     // Exactly one heap word gap right before dense prefix end; the filler obj
> 1458:     // will extend to next region.
> 1459:     const size_t obj_len = 2; // min-obj-size

It may make sense to use `CollectedHeap::min_fill_size()` for the `2` magic constants here even if there is that assert at the top of the method (and other code below assumes that). After all, the code puts in an object of `min_fill_size()`.
Similarly I think the comment next to this line is not that useful, the important point is that the filler object that is about to be put in is of size `min_fill_size()`, not `min_obj_size()` (although they are the same).
Then the comment can also go away because it's self-describing imo.

src/hotspot/share/gc/parallel/psParallelCompact.cpp line 2549:

> 2547:   HeapWord* const space_bottom = space(space_id)->bottom();
> 2548: 
> 2549:   // Check if it's the first region in this space.

I think this comment is superfluous.

src/hotspot/share/gc/parallel/psParallelCompact.cpp line 2566:

> 2564:       if (!mbm->is_obj_beg(beg_bit) && !mbm->is_obj_end(beg_bit - 1)) {
> 2565:         beg_addr = mbm->find_obj_beg(beg_addr, end_addr);
> 2566:       }

I am not sure this refactoring improves upon the previous code. It adds an additional block level in the code, and removes the descriptive method name.
Actually this and the previous hunk is simply inlining of `dead_space_crosses_boundary`.

Not insisting on reverting this change, but I prefer putting more complicated conditions into descriptive helpers.

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

PR Review: https://git.openjdk.org/jdk/pull/17822#pullrequestreview-1908333386
PR Review Comment: https://git.openjdk.org/jdk/pull/17822#discussion_r1507390615
PR Review Comment: https://git.openjdk.org/jdk/pull/17822#discussion_r1507243353
PR Review Comment: https://git.openjdk.org/jdk/pull/17822#discussion_r1507388927
PR Review Comment: https://git.openjdk.org/jdk/pull/17822#discussion_r1507405363
PR Review Comment: https://git.openjdk.org/jdk/pull/17822#discussion_r1507398132
PR Review Comment: https://git.openjdk.org/jdk/pull/17822#discussion_r1507385567
PR Review Comment: https://git.openjdk.org/jdk/pull/17822#discussion_r1507423208
PR Review Comment: https://git.openjdk.org/jdk/pull/17822#discussion_r1507434042