RFR: 8335902: Parallel: Refactor VM_ParallelGCFailedAllocation and VM_ParallelGCSystemGC [v3]

[Guoxiong Li]

On Wed, 10 Jul 2024 20:29:37 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Similar cleanup as https://github.com/openjdk/jdk/pull/19056 but in Parallel. As a result, the corresponding code in `SerialHeap` and `ParallelScavengeHeap` share much similarity.
>> 
>> The easiest way to review is to start from these two VM operations, `VM_ParallelCollectForAllocation` and `VM_ParallelGCCollect` and follow the new code directly, where one can see how allocation-failure triggers various GCs with different collection efforts.
>> 
>> Test: tier1-6; perf-neural for dacapo, specjvm2008, specjbb2015 and cachestresser.
>
> 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 pgc-vm-operation
>  - pgc-vm-operation

Nice refactor.

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 434:

> 432: void ParallelScavengeHeap::do_full_collection_no_gc_locker(bool clear_all_soft_refs) {
> 433:   bool maximum_compaction = clear_all_soft_refs;
> 434:   PSParallelCompact::invoke(maximum_compaction);

The parameter `maximum_heap_compaction` of the method `PSParallelCompact::invoke` was changed to `clear_all_soft_refs` in [JDK-8334445](https://git.openjdk.org/jdk/pull/19763), so the variable `maximum_compaction` seems not necessary here.

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 443:

> 441:   if (result == nullptr && !is_tlab) {
> 442:     // auto expand inside
> 443:     result = old_gen()->allocate(size);

If we expand the generation in the method `PSOldGen::allocate`. I think it is good to rename the method to `expand_and_allocate` (just like `TenuredGeneration::expand_and_allocate` in SerialGC). It is better to be polished at a followup issue.

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 446:

> 444:   }
> 445:   return result;   // Could be null if we are out of space.
> 446: }

I notice the method `PSOldGen::allocate` can expand the size of the old gen, but the method `PSYoungGen::allocate` can't expand the size of the young gen. It is similar to a bug [1] in Serial. Fortunately, the size of the young generation can be resized during Parallel GC if the option `UseAdaptiveSizePolicy` is `true`. When the `UseAdaptiveSizePolicy` is set to `false` manually by the user, I suspect it is a bug in Parallel because of the unexpanded young generation size.

[1] https://bugs.openjdk.org/browse/JDK-8333386

src/hotspot/share/gc/parallel/parallelScavengeHeap.cpp line 478:

> 476: 
> 477:     const bool clear_all_soft_refs = true;
> 478:     do_full_collection_no_gc_locker(clear_all_soft_refs);

If the young collection succeeded in method `collect_at_safepoint`. The normal full collection won't run in `collect_at_safepoint`. If the successful young collection didn't release any memory (or only released little memory but not enough for allocation), the allocation in line 462 will fail too. Then a full collection with maximum compaction will be run. It is strange. In my opinion, I think the steps look like below:

1. allocation
2. young collection
3. allocation
4. normal full collection
5. allocation
6. maximum full collection
7. allocation
8. OOM

But in current patch, the step 4-5 may be skipped.

src/hotspot/share/gc/parallel/parallelScavengeHeap.hpp line 114:

> 112: 
> 113:   // Perform a full collection
> 114:   void do_full_collection(bool clear_all_soft_refs) override;

The comment seems redundant.

src/hotspot/share/gc/parallel/psScavenge.cpp line 232:

> 230: // Note that this method should only be called from the vm_thread while
> 231: // at a safepoint!
> 232: bool PSScavenge::invoke() {

Nice removal. It is strange to run a full collection in `PSScavenge` before.

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

Changes requested by gli (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20077#pullrequestreview-2172099153
PR Review Comment: https://git.openjdk.org/jdk/pull/20077#discussion_r1674132961
PR Review Comment: https://git.openjdk.org/jdk/pull/20077#discussion_r1674390370
PR Review Comment: https://git.openjdk.org/jdk/pull/20077#discussion_r1674247874
PR Review Comment: https://git.openjdk.org/jdk/pull/20077#discussion_r1674379967
PR Review Comment: https://git.openjdk.org/jdk/pull/20077#discussion_r1674344233
PR Review Comment: https://git.openjdk.org/jdk/pull/20077#discussion_r1674384804