RFR: 8267924: Misleading G1 eager reclaim detail logging

[Stefan Johansson]

On Fri, 4 Jun 2021 10:42:48 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.cpp line 141:
>> 
>>> 139:     // are currently allocated into.
>>> 140:     uint region_idx = r->hrm_index();
>>> 141:     if (!g1h->is_humongous_reclaim_candidate(region_idx)) {
>> 
>> Fwiw, the reason why the second clause has been removed is because it is useless: G1 does not add remembered set entries during gc, so if the remembered set has been empty before, it will be empty again.
>> Humongous regions with a large enough remembered set at the start are never candidates, and ones that are candidates have all of their outstanding references checked and if that is the case, their candidate state revoked, which is reflected in the `is_humongous_reclaim_candidate` check.
>
> Could you update the comments a bit? Feels like they will be outdated after this change.

I think it would be nice to somehow highlight that this is no longer just a "candidate". When reading the code it looks like we early out `if (!candidate)` and that suggests that we then should check the candidate if it can be reclaimed. But that is already done during the GC, so anything still a candidate is reclaimable. What do you think about adding a helper to the closure: 

// Any humongous object still a reclaim candidate at this point can be reclaimed.
bool should_reclaim(uint region) { 
  return G1CollectedHeap::heap()->is_humongous_reclaim_candidate(region_idx);
}

Maybe with a bit more thought through comment.

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

PR: https://git.openjdk.java.net/jdk/pull/4305