-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8357053: ZGC: Improved utility for ZPageAge #25251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 Welcome back jsikstro! A progress list of the required criteria for merging this PR into |
|
@jsikstro 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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
|
/label remove hotspot |
|
@jsikstro |
|
@jsikstro |
|
/contributor add @xmas92 |
|
@jsikstro |
Webrevs
|
|
copyright-year update is missing on at least 2 files |
stefank
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of questions before fully reviewing this?
stefank
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but I found a couple nits.
kstefanj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really nice. As discussed offline, adding a gtest to make sure that the ranges cover the expected ages could be an additional nice addition.
|
Thank you for the reviews! |
|
Going to push as commit 1c72b35. |
Hello,
This RFE improves utility for converting to/from, iterating over and defining structures that are indexed using the
ZPageAgetype.Converting to/from ZPageAge and its underlying type (uint8_t, often just uint) is currently done via using static_cast. This works fine because sane values are converted in all use cases. However, to make conversion safer (and also more readable), I propose we add a
to_zpageageand a correspondinguntypethat checks that the conversion is valid. Such conversion methods should be used instead of callingstatic_cast<uint/ZPageAge>.We currently define a value called
ZPageAgeMax, which is defined asstatic_cast<uint>(ZPageAge::old). The majority of places that use this value actualy useZPageAgeMax + 1, which is equivalent to the number of ages. Instead, I propose we define and use a value that represents the number of possible ages, calledZPageAgeCount.Lastly, to make iterating over ages more accessible, I propose we create an intreface of enum iterators of ZPageAge. This will also create a foundation for generating values that require a ZPageAge in the future. Since the end of the enum iterators are exclusive, I've opted to use the following value as end for the iterators:
I see us using either this or a sentinel/dummy value at the end of the enum class, but I prefer having a value similar to
ZPageAgeLastPlusOneover a dummy value.Testing:
Progress
Issue
Reviewers
Contributors
<aboldtch@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25251/head:pull/25251$ git checkout pull/25251Update a local copy of the PR:
$ git checkout pull/25251$ git pull https://git.openjdk.org/jdk.git pull/25251/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25251View PR using the GUI difftool:
$ git pr show -t 25251Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25251.diff
Using Webrev
Link to Webrev Comment