On Fri, 10 May 2024 08:55:36 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 seven commits: > > - Merge branch 'master' into s1-do-collect > - review > - Merge branch 'master' into s1-do-collect > - merge > - review > - Merge branch 'master' into s1-do-collect > - s1-do-collect Changes requested by iwalulya (Reviewer). src/hotspot/share/gc/serial/serialHeap.cpp line 557: > 555: return result; > 556: } > 557: Would be nice to add a comment here to indicate that the previous collection could have shrunk the heap. src/hotspot/share/gc/serial/serialHeap.cpp line 714: > 712: > 713: void SerialHeap::do_full_collection_no_gc_locker(bool > clear_all_soft_refs) { > 714: IsSTWGCActiveMark gc_active_mark; `IsSTWGCActiveMark active_gc_mark;`in`do_young_collection_no_gc_locker`, just choose one and be consistent with it src/hotspot/share/gc/serial/serialHeap.cpp line 907: > 905: > 906: void SerialHeap::print_tracing_info() const { > 907: // Nothing What is the `Nothing` supposed to convey here? src/hotspot/share/gc/serial/serialHeap.hpp line 117: > 115: void do_full_collection_no_gc_locker(bool clear_all_soft_refs); > 116: > 117: void collect_at_safepoint_no_gc_locker(bool full); I am not very convinced by the naming of the methods with the "no_gc_locker" constraint. But I guess it is following same convention as "*at_safepoint" method naming. ------------- PR Review: https://git.openjdk.org/jdk/pull/19056#pullrequestreview-2062384283 PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1604380445 PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1604389765 PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1604390627 PR Review Comment: https://git.openjdk.org/jdk/pull/19056#discussion_r1604388902