RFR: 8331557: Serial: Refactor SerialHeap::do_collection [v2]

[Guoxiong Li]

On Fri, 3 May 2024 12:49:03 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> It's probably easier to read the new code directly. The two classes in `serialVMOperations` serve as entrance points to invoke young/full GCs. Some previously hidden decisions are made more obvious, e.g. if a young-gc fails (or will probablly fail), fallback to full-gc.
>> 
>> Additionally, `StatRecord` is removed, because this kind of info-aggregation should be done outsite VM (by third-party tool).
>> 
>> Test: tier1-6
>
> Albert Mingkun Yang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit:
> 
>   s1-do-collect

Nice refactor.

src/hotspot/share/gc/serial/serialHeap.cpp line 442:

> 440: }
> 441: 
> 442: bool SerialHeap::do_young_gc(DefNewGeneration* young_gen, bool clear_soft_refs) {

The parameter `DefNewGeneration* young_gen` is not necessary. We can use the field `SerialHeap::_young_gen` directly.

src/hotspot/share/gc/serial/serialHeap.cpp line 461:

> 459:   if (should_verify && VerifyBeforeGC) {
> 460:     prepare_for_verify();
> 461:     Universe::verify("Before GC");

May the prefix of the verification log be better to specify the minor or full GC? Such as `Before Minor GC` here.

src/hotspot/share/gc/serial/serialHeap.cpp line 463:

> 461:     Universe::verify("Before GC");
> 462:   }
> 463:   gc_prologue(false);

The parameter `full` of the method `SerialHeap::gc_prologue` doesn't been used. Seems a leftover of [JDK-8323993](https://bugs.openjdk.org/browse/JDK-8323993).

src/hotspot/share/gc/serial/serialHeap.cpp line 468:

> 466:   gen->stat_record()->accumulated_time.stop();
> 467: 
> 468:   update_gc_stats(gen, full);

The method `update_gc_stats` is only used by young-gen to sample the promoted size. It is good to rename and simplify the related code. I filed https://bugs.openjdk.org/browse/JDK-8331684 to follow up.

src/hotspot/share/gc/serial/serialHeap.cpp line 660:

> 658:   }
> 659:   do_full_collection_no_gc_locker(clear_soft_refs);
> 660: }

Please note the difference between the previous `SerialHeap::do_collection` and `SerialHeap::collect_at_safepoint_no_gc_locker` here. The previous `SerialHeap::do_collection` may invoke full GC according to the method `SerialHeap::should_do_full_collection` even the young GC succeeded. But `SerialHeap::collect_at_safepoint_no_gc_locker` only invokes full GC when the young GC failed (because of failed promotion). Such change makes the `SerialHeap::should_do_full_collection` has no user. If the behaviour of the `SerialHeap::collect_at_safepoint_no_gc_locker` is your intention, I think it is good to remove `SerialHeap::should_do_full_collection`.

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

Changes requested by gli (Committer).

PR Review: https://git.openjdk.org/jdk/pull/19056#pullrequestreview-2039185857
PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1589844934
PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1589860506
PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1589859847
PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1589857649
PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1589863014