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

[Albert Mingkun Yang]

On Wed, 15 May 2024 10:20:55 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> 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
>
> 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`.

Since `RegionSize` is always some power of 2, `>= BitsPerWord` is sufficient to verify the bit for region-start is word-aligned.

(It's mostly to guard against too small `Log2RegionSize`.)

> 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?

I believe it's handled in cld-processing, `ClassLoaderDataGraph::cld_do(&cld_closure);`.

(Actually, we also use dedicated filler klasses, so this comment is not super accurate. Additionally, the method was named `adjust_roots` and it's not obvious why it must be called before `compact `, hence the comment. With the new mark-compact algo, it's kind of expected that adjust goes before compact, so the usefulness of this comment is limited. Maybe it's better to remove it completely.)

> 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?

Maybe there is a misleading re `destination` here.

For non-spliting regions, `destination` is the destination for the first live word in the current region.

> 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?

This and the following question are closely related. The troublesome scenario is that the region for a worker starts with dead-space, which is continuation from proceeding region. In this case, we wanna the proceeding worker creates a single large filler that spans over to regions belong to next worker.

> 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`.

The early-return is needed to ensure `cast_to_oop(live_start)->size()` is safe.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19101#discussion_r1602207985
PR Review Comment: https://git.openjdk.org/jdk/pull/19101#discussion_r1602255136
PR Review Comment: https://git.openjdk.org/jdk/pull/19101#discussion_r1602231939
PR Review Comment: https://git.openjdk.org/jdk/pull/19101#discussion_r1602236352
PR Review Comment: https://git.openjdk.org/jdk/pull/19101#discussion_r1602237720