On Fri, 3 May 2024 12:49:03 GMT, Albert Mingkun Yang <ay...@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
