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

[Albert Mingkun Yang]

On Sat, 4 May 2024 02:39:38 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 pull request now contains one commit:
>> 
>>   s1-do-collect
>
> 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.

Other `Universe::verify("` seems to not distinguish minor/major.

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

True; can probably fixed in a followup cleanup.

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

Removed `should_do_full_collection`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1590740631
PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1590740947
PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1590741518