8217327: G1 Post-Cleanup region liveness printing should not print out-of-date efficiency #2217
Conversation
|
👋 Welcome back jaokim! A progress list of the required criteria for merging this PR into |
Webrevs
|
| type, p2i(bottom), p2i(end), | ||
| used_bytes, prev_live_bytes, next_live_bytes, "-", | ||
| remset_bytes, remset_type, strong_code_roots_bytes); | ||
| snprintf(gc_efficiency, G1PPRL_DOUBLE_FORMAT_LEN+1, G1PPRL_DOUBLE_H_FORMAT, "-"); |
There was a problem hiding this comment.
snprintf is fine with me too, but I had imagined something like this:
FormatBuffer<> efficiency(""); // maybe better name this
if (gc_eff < 0.0) {
efficiency.append("-");
} else {
efficiency.append(G1PPRL_DOUBLE_H_FORMAT, gc_eff);
}
and in the log_trace use %s and efficiency.buffer(). That seems a lot easier to understand (for me) than wrangling with snprintf.
Maybe there is a reason to not use this?
Also, I am not sure that using G1PPRL_DOUBLE_H_FORMAT for this snprintf here to print only - is really correct. I would have naively have expected to require %s.
There was a problem hiding this comment.
Again, you're absolutely right.
I looked for a FormatBuffer class in the codebase when you mentioned it (kind of what I wanted from the beginning). For whatever reason I couldn't find one (typo, temporary blindness, ignorance??). So I went with snprintf (even though I don't particular like it).
I'll fix this. Sorry for all the bother.
|
|
@jaokim 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 252 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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@tschatzl, @kstefanj) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
Thanks @tschatzl for review and comments! |
|
Thanks for review @kstefanj. |
|
/integrate |
|
/sponsor |
openjdk
bot
added
integrated
|
@tschatzl @jaokim Since your change was applied there have been 256 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 50f9a70. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Description
This fix addresses the issue where gc-efficiency is printed incorrectly when logging post-marking and post-cleanup. The gc-efficiency is calculated in the end of the marking phase, to be logged in the post-cleanup section. It is however not reset, meaning that next phase's post-marking log will show the old value.
HeapRegion::note_start_of_marking()Note: there is a sister issue that moves the post-cleanup printing to a later stage. Without this fix, the logging will still be incorrect, so both fixes are needed. See: JDK-8260042: G1 Post-cleanup liveness printing occurs too early
This fix has been tested together with the above mentioned fix.
Example
This is what logging like after fix has been applied.
Testing
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2217/head:pull/2217$ git checkout pull/2217