8274178: Occupancy value in logging and JFR event is inaccurate in G1IHOPControl

[caoman]

Hi all,

Could we have reviews for this minor fix for logging and JFR event occupancy number for G1? See https://bugs.openjdk.java.net/browse/JDK-8274178 for more details.

This fix is contributed by colleague Jonathan Joo jonathanjoo@google.com.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8274178: Occupancy value in logging and JFR event is inaccurate in G1IHOPControl

Contributors

• Jonathan Joo <jonathanjoo@google.com>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5762/head:pull/5762
$ git checkout pull/5762

Update a local copy of the PR:
$ git checkout pull/5762
$ git pull https://git.openjdk.java.net/jdk pull/5762/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5762

View PR using the GUI difftool:
$ git pr show -t 5762

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5762.diff

[bridgekeeper]

👋 Welcome back manc! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@caoman The following label will be automatically applied to this pull request:

• hotspot-gc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[caoman]

/contributor add Jonathan Joo jonathanjoo@google.com

[openjdk]

@caoman Could not parse Jonathan Joo jonathanjoo@google.com as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

• /contributor add @openjdk-bot
• /contributor add duke
• /contributor add J. Duke <duke@openjdk.org>

[caoman]

/contributor add Jonathan Joo jonathanjoo@google.com

[openjdk]

@caoman
Contributor Jonathan Joo <jonathanjoo@google.com> successfully added.

[mlbridge]

Webrevs

• 00: Full (b0f665f9)

[tschatzl]

I am not completely sure this change is correct: the target occupancy is the occupancy we want to reach, which is

void G1Policy::record_new_heap_size(uint new_number_of_regions) {
[...]
  _ihop_control->update_target_occupancy(new_number_of_regions * HeapRegion::GrainBytes);
}

i.e. the current committed amount of regions.

I think it is interesting for the user to see what the current usage (fraction of target occupancy, i.e. committed regions) is when marking starts.

The change has a valid point though: since get_conc_mark_start_threshold is compared towards the non-young occupancy. What about providing that information additionally instead of replacing the existing info?

Or alternatively only provide information compared against non young capacity (i.e. the _target_occupancy is, as mentioned, against the whole committed memory, not just non-young).

Thanks,
Thomas

[jjoo172]

Hi @jjoo172, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user jjoo172 for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

• I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

[bridgekeeper]

@caoman This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@caoman This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.