On Sun, 18 May 2025 17:21:40 GMT, Guoxiong Li <g...@openjdk.org> wrote:
> so that we can do the check after all the GCs
Well, not really. In the old impl, `GCOverheadChecker::check_gc_overhead_limit`
calls `set_gc_overhead_limit_exceeded` only for full-gc.
> But now you only use check_gc_overhead_limit in
> ParallelScavengeHeap::satisfy_failed_allocation. I suspect whether it can
> check the gc overhead limit accurately.
I believe so. In the old impl, we don't check gc-overhead for explicit gcs.
Only allocation-failure caused gcs are interesting, which all go through
`satisfy_failed_allocation`.
// Ignore explicit GC's. Exiting here does not set the flag and
// does not reset the count.
if (GCCause::is_user_requested_gc(gc_cause) ||
GCCause::is_serviceability_requested_gc(gc_cause)) {
return;
}
> src/hotspot/share/gc/parallel/psYoungGen.cpp line 268:
>
>> 266: size_t original_committed_size = virtual_space()->committed_size();
>> 267:
>> 268: while (true) {
>
> The `while` statement only runs once. May we find a better way to refactor
> the code?
I don't see an easy to re-structure the code while keeping all the relevant
logic in the current context. I added some comments; check if it makes the flow
easier to follow.
> src/hotspot/share/gc/parallel/psYoungGen.cpp line 334:
>
>> 332: assert(from_space()->capacity_in_bytes() ==
>> to_space()->capacity_in_bytes(), "inv");
>> 333: const size_t current_survivor_size =
>> from_space()->capacity_in_bytes();
>> 334: assert(max_gen_size() > 2 * current_survivor_size, "inv");
>
> Should this assertion be changed to `assert(max_gen_size() >
> current_eden_size + 2 * current_survivor_size, "inv");` ?
Revised; needs to be `>=` though.
> src/hotspot/share/gc/parallel/psYoungGen.cpp line 379:
>
>> 377: // We usually resize young-gen only after a successful young-gc.
>> However, in emergency state, we wanna expand young-gen to its max-capacity.
>> 378: // Young-gen should be empty normally after a full-gc.
>> 379: if (eden_space()->is_empty() && to_space()->is_empty()) {
>
> Why don't you test the `from space` here? And actually, if the `eden space`
> is empty, the `from space` and `to space` are empty too, because the objects
> are firstly moved to `eden space`. See the method
> `PSParallelCompact::summary_phase` for more information. So here, you only
> need to test whether the `eden space` is empty.
Added checking for `from_space`.
If all live-objs don't fit in old-gen, leftovers will be kept in its own space.
// Summarize the remaining spaces in the young gen. The initial target space
// is the old gen. If a space does not fit entirely into the target, then the
// remainder is compacted into the space itself and that space becomes the new
// target.
> src/hotspot/share/gc/parallel/psYoungGen.cpp line 487:
>
>> 485:
>> 486: void PSYoungGen::resize_spaces(size_t requested_eden_size,
>> 487: size_t requested_survivor_size) {
>
> You remove some `trace` level logs in this method. Please confirm whether it
> is your intent?
Yes; when testing/developing, I don't find them to be very useful.
> src/hotspot/share/gc/shared/adaptiveSizePolicy.hpp line 48:
>
>> 46: // Default: 100ms.
>> 47: static constexpr double MinGCDistanceSecond = 0.100;
>> 48: static_assert(MinGCDistanceSecond >= 0.001, "inv");
>
> The`MinGCDistanceSecond` is just a contant; the static assertion seems
> unnecessary?
It's more to convey the intend that this number have a lower bound if future
changes want to lower it.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094893944
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094867831
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094870461
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094882156
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094882750
PR Review Comment: https://git.openjdk.org/jdk/pull/25000#discussion_r2094883781
