8380526: G1: Remove "last young" use for the Prepare Mixed GC#30336
8380526: G1: Remove "last young" use for the Prepare Mixed GC#30336tschatzl wants to merge 13 commits intoopenjdk:masterfrom
Conversation
Hi all, please review this change that makes `G1CollectorState` get the concurrent cycle state directly from `G1ConcurrentMark[Thread]` instead of, in multiple places set the current state separately. This is kind of error-prone, as https://bugs.openjdk.org/browse/JDK-8378952, which this change also implicitly fixes, shows. Other notes: * I expanded the `ServiceState` in `G1ConcurrentMarkThread` instead of adding a separate variable holding whether this is a full or undo concurrent cycle. This avoids potential races between those two too. * be more consistent with `Concurrent Cycle` (the whole thing) vs. `Concurrent Marking` throughout * renamed the "clear bitmap" state to "reset for next cycle` since clearing the bitmap is only one part of that subphase of the concurrent cycle. * made concurrent cycle state queries always go through `G1CollectorState` * fixed `in_something` to `is_in_something` for `G1CollectorState` as per style guideline * extended evacuation failure injection to include reset for next cycle for the `G1GCAllocationFailureALotDuringConcMark` value; I thought it does not hurt and it already covered both mark and rebuild or scrub Testing: tier1-5 Thanks, Thomas
Hi all, please review this change that changes `G1CollectorState` to use a single state variable for GC pause phasing instead of multiple bools. This reduces clutter checking validity of these flags, and imo makes the state very explicit with the usual advantages in readability. This is based on PR#30144, so please review that one first. Testing: tier1-5, gha Thanks, Thomas
…ause_type()` because it covers all types of pauses now
|
👋 Welcome back tschatzl! A progress list of the required criteria for merging this PR into |
|
@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 12 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 |
|
The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout submit/8380526-remove-last-young
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push |
|
@tschatzl this pull request can not be integrated into git checkout submit/8380526-remove-last-young
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
* reorder elements in Pause type to correspond to typical execution (i.e. Prepare Mixed last)
|
|
||
| // Phase getters | ||
| bool is_in_young_only_phase() const { return _phase == Phase::YoungNormal || _phase == Phase::YoungConcurrentStart || _phase == Phase::YoungLastYoung; } | ||
| bool is_in_young_only_phase() const { return _phase == Phase::YoungNormal || _phase == Phase::YoungConcurrentStart || _phase == Phase::YoungPrepareMixed; } |
There was a problem hiding this comment.
Maybe the impl can be broken into multiple lines.
There was a problem hiding this comment.
_phase <= Phase::YoungPrepareMixed; or do we deliberately choose not to rely on the enum ordering?
There was a problem hiding this comment.
Maybe the impl can be broken into multiple lines
I was going to do this in PR#30399
_phase <= Phase::YoungPrepareMixed; or do we deliberately choose not to rely on the enum ordering?
Deliberate, after changing it to explicit values in some earlier change.
|
Thanks @albertnetymk @walulyai for your reviews |
|
Going to push as commit d0d85cd.
Your commit was automatically rebased without conflicts. |
Hi all,
please review this change that removes the "Last Young" nomenclature for the "Prepare Mixed" gc. The additional name just confuses.
Testing: gha
Thanks,
Thomas
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30336/head:pull/30336$ git checkout pull/30336Update a local copy of the PR:
$ git checkout pull/30336$ git pull https://git.openjdk.org/jdk.git pull/30336/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30336View PR using the GUI difftool:
$ git pr show -t 30336Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30336.diff
Using Webrev
Link to Webrev Comment