On Wed, 10 Jul 2024 20:29:37 GMT, Albert Mingkun Yang <ay...@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
