RFR: 8329203: Parallel: Investigate Mark-Compact for Full GC to decrease memory usage [v4]

[Guoxiong Li]

On Mon, 13 May 2024 09:11:38 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Refactor Parallel full-gc to use the same algorithm (mark-compact) as Serial and G1 full-GC. This removes the obj-end bitmap. When GC threads are few, the old implementation can be more efficient because it requires fewer heap iterations. The new full-GC implementation, on the other hand, is more scalable because it introduces more phases (`forward_to_new_addr` and `adjust_pointers`) that can partition work effectively.
>> 
>> The diff is rather large, so reading the new code directly from `invoke_no_policy` is probably easier.
>> 
>> Test: tier1-6; some improvement in Dacapo-h2, CacheStresser, but no difference in specjbb2015, specjvm2008.
>
> 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 seven additional commits since the last revision:
> 
>  - Merge branch 'master' into pgc-full-gc
>  - review
>  - remove-comment
>  - Merge branch 'master' into pgc-full-gc
>  - review
>  - Merge branch 'master' into pgc-full-gc
>  - pgc-full-gc

Sorry for any delay. Several suggestions/questions.

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

> 102: const size_t ParallelCompactData::Log2RegionSize  = 16; // 64K words
> 103: const size_t ParallelCompactData::RegionSize      = (size_t)1 << Log2RegionSize;
> 104: static_assert(ParallelCompactData::RegionSize >= BitsPerWord, "region-start bit word-aligned");

The `ParallelCompactData::RegionSize >= BitsPerWord` seems not to test `word-aligned`.

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

> 1290:     // adjust_pointers() updates Universe::_intArrayKlass which is
> 1291:     // needed by the compaction for filling holes in the dense prefix.
> 1292:     adjust_pointers();

I can't find the code which updates `Universe::_intArrayKlass` according to the comment. Is the comment a useless leftover during previous refactor?

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

> 1837:             if (new_addr != cur_addr) {
> 1838:               cm->preserved_marks()->push_if_necessary(obj, obj->mark());
> 1839:               obj->forward_to(cast_to_oop(new_addr));

The `live_words` is firstly initialized as `partial_obj_size` of the current region. But then it is added to the `destination` to construct the new address. The `destination` is a precise location of the `dest-region` instead of current region, so the new address may be not right when `partial_obj_size` is not zero. But I don't know why all the tests passed. Do I miss anything?

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

> 2083: 
> 2084:   // end_addr is inclusive to handle regions starting with dead space.
> 2085:   while (cur_addr <= end_addr) {

When the `cur_addr` is equal than `end_addr`, the corresponding region would be handled by another thread. Why does this thread need to handle it? Do I miss anything?

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

> 2090:       if (cur_addr != start_addr || worker_id == 0) {
> 2091:         fill_range_in_dense_prefix(cur_addr, live_start);
> 2092:       }

The regions are spilt by the method `split_regions_for_worker`. Why do we need the conditional statement here?

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

> 2095:       break;
> 2096:     }
> 2097:     assert(bitmap->is_marked(live_start), "inv");

Does this conditional break statement just let the assertion pass? If so, what about removing the conditional statement and changing the assertion to `bitmap->is_marked(live_start) || live_start == prefix_end`.

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

Changes requested by gli (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19101#pullrequestreview-2057556023
PR Review Comment: https://git.openjdk.org/jdk/pull/19101#discussion_r1601368366
PR Review Comment: https://git.openjdk.org/jdk/pull/19101#discussion_r1602019810
PR Review Comment: https://git.openjdk.org/jdk/pull/19101#discussion_r1601911117
PR Review Comment: https://git.openjdk.org/jdk/pull/19101#discussion_r1602098938
PR Review Comment: https://git.openjdk.org/jdk/pull/19101#discussion_r1602069208
PR Review Comment: https://git.openjdk.org/jdk/pull/19101#discussion_r1602090280