8267924: Misleading G1 eager reclaim detail logging #4305
Conversation
|
👋 Welcome back tschatzl! A progress list of the required criteria for merging this PR into |
| g1h->is_humongous_reclaim_candidate(region_idx), | ||
| obj->is_typeArray() | ||
| ); | ||
| if (!g1h->is_humongous_reclaim_candidate(region_idx)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Could you update the comments a bit? Feels like they will be outdated after this change.
There was a problem hiding this comment.
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.
Webrevs
|
| g1h->is_humongous_reclaim_candidate(region_idx), | ||
| obj->is_typeArray() | ||
| ); | ||
| if (!g1h->is_humongous_reclaim_candidate(region_idx)) { |
There was a problem hiding this comment.
Could you update the comments a bit? Feels like they will be outdated after this change.
| g1h->is_humongous_reclaim_candidate(region_idx), | ||
| obj->is_typeArray() | ||
| ); | ||
| if (!g1h->is_humongous_reclaim_candidate(region_idx)) { |
There was a problem hiding this comment.
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.
|
@tschatzl This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 77 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
openjdk
bot
added
integrated
|
@tschatzl Since your change was applied there have been 83 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 15715a8. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi,
can I have reviews for this change that brushes up
gc+humongouslogging?In particular, during review of some logs several issues were found with the logging.
Both the "Live humongous region" and "Dead humongous region" logs
This change modifies the log output to try to give useful information: first, g1 prints information for all humongous regions when deciding whether something is a candidate. This means that the printed information is current. At the end of gc only reclaimed regions are printed, repeating the information from the printout at the beginning does not give more information.
E.g. a sample output from the test case
First, all humongous regions
Later only reclaimed ones:
Instead of currently:
or
This unfortunately kind of invalidates some websites trying to explain these log messages, but I still prefer to have useful output.
Thanks,
Thomas
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4305/head:pull/4305$ git checkout pull/4305Update a local copy of the PR:
$ git checkout pull/4305$ git pull https://git.openjdk.java.net/jdk pull/4305/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4305View PR using the GUI difftool:
$ git pr show -t 4305Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4305.diff