On Sat, 4 May 2024 02:39:38 GMT, Guoxiong Li <[email protected]> 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
