jdk
jdk copied to clipboard
openjdk
8137022: Concurrent refinement thread adjustment and (de-)activation suboptimal
8137022: Concurrent refinement thread adjustment and (de-)activation suboptimal
8155996: Improve concurrent refinement green zone control
8134303: Introduce -XX:-G1UseConcRefinement
Please review this change to the control of concurrent refinement.
This new controller takes a different approach to the problem, addressing a number of issues.
The old controller used a multiple of the target number of cards to determine the range over which increasing numbers of refinement threads should be activated, and finally activating mutator refinement. This has a variety of problems. It doesn't account for the processing rate, the rate of new dirty cards, or the time available to perform the processing. This often leads to unnecessary spikes in the number of running refinement threads. It also tends to drive the pending number to the target quickly and keep it there, removing the benefit from having pending dirty cards filter out new cards for nearby writes. It can't delay and leave excess cards in the queue because it could be a long time before another buffer is enqueued.
The old controller was triggered by mutator threads enqueing card buffers, when the number of cards in the queue exceeded a threshold near the target. This required a complex activation protocol between the mutators and the refinement threads.
With the new controller there is a primary refinement thread that periodically estimates how many refinement threads need to be running to reach the target in time for the next GC, along with whether to also activate mutator refinement. If the primary thread stops running because it isn't currently needed, it sleeps for a period and reevaluates on wakeup. This eliminates any involvement in the activation of refinement threads by mutator threads.
The estimate of how many refinement threads are needed uses a prediction of time until the next GC, the number of buffered cards, the predicted rate of new dirty cards, and the predicted refinement rate. The number of running threads is adjusted based on these periodically performed estimates.
This new approach allows more dirty cards to be left in the queue until late in the mutator phase, typically reducing the rate of new dirty cards, which reduces the amount of concurrent refinement work needed.
It also smooths out the number of running refinement threads, eliminating the unnecessarily large spikes that are common with the old method. One benefit is that the number of refinement threads (lazily) allocated is often much lower now. (This plus UseDynamicNumberOfGCThreads mitigates the problem described in JDK-8153225.)
This change also provides a new method for calculating for the number of dirty cards that should be pending at the start of a GC. While this calculation is conceptually distinct from the thread control, the two were significanly intertwined in the old controller. Changing this calculation separately and first would have changed the behavior of the old controller in ways that might have introduced regressions. Changing it after the thread control was changed would have made it more difficult to test and measure the thread control in a desirable configuration.
The old calculation had various problems that are described in JDK-8155996. In particular, it can get more or less stuck at low values, and is slow to respond to changes.
The old controller provided a number of product options, none of which were very useful for real applications, and none of which are very applicable to the new controller. All of these are being obsoleted.
-XX:-G1UseAdaptiveConcRefinement
-XX:G1ConcRefinementGreenZone=
The new controller could use G1ConcRefinementGreenZone to provide a fixed value for the target number of cards, though it is poorly named for that.
A configuration that was useful for some kinds of debugging and testing was to disable G1UseAdaptiveConcRefinement and set g1ConcRefinementGreenZone to a very large value, effectively disabling concurrent refinement. To support this use case with the new controller, the -XX:-G1UseConcRefinement diagnostic option has been added (see JDK-8155996).
The other options are meaningless for the new controller.
Because of these option changes, a CSR and a release note need to accompany this change.
Testing: mach5 tier1-6 various performance tests. local (linux-x64) tier1 with -XX:-G1UseConcRefinement
Performance testing found no regressions, but also little or no improvement with default options, which was expected. With default options most of our performance tests do very little concurrent refinement. And even for those that do, while the old controller had a number of problems, the impact of those problems is small and hard to measure for most applications.
When reducing G1RSetUpdatingPauseTimePercent the new controller seems to fare better, particularly when also reducing MaxGCPauseMillis. specjbb2015 with MaxGCPauseMillis=75 and G1RSetUpdatingPauseTimePercent=3 (and other options held constant) showed a statistically significant improvement of about 4.5% for critical-jOPS. Using the changed controller, the difference between this configuration and the default is fairly small, while the baseline shows significant degradation with the more restrictive options.
For all tests and configurations the new controller often creates many fewer refinement threads.
/issue add 8155996 /issue add 8134303
/csr
Progress
- [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [ ] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change requires a CSR request to be approved
Issues
- JDK-8137022: Concurrent refinement thread adjustment and (de-)activation suboptimal
- JDK-8155996: Improve concurrent refinement green zone control
- JDK-8134303: Introduce -XX:-G1UseConcRefinement
- JDK-8294206: Concurrent refinement thread adjustment and (de-)activation suboptimal (CSR)
Reviewers
- Stefan Johansson (@kstefanj - Reviewer) ⚠️ Review applies to 9a735bc0
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10256/head:pull/10256
$ git checkout pull/10256
Update a local copy of the PR:
$ git checkout pull/10256
$ git pull https://git.openjdk.org/jdk pull/10256/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10256
View PR using the GUI difftool:
$ git pr show -t 10256
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10256.diff
:wave: Welcome back kbarrett! 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@kimbarrett
Adding additional issue to issue list: 8155996: Improve concurrent refinement green zone control.
@kimbarrett
Adding additional issue to issue list: 8134303: Introduce -XX:-G1UseConcRefinement.
@kimbarrett has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@kimbarrett please create a CSR request for issue JDK-8137022 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.
@kimbarrett The following label will be automatically applied to this pull request:
hotspot
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.
Webrevs
- 07: Full (0c2b4c69)
- 06: Full - Incremental (f6662698)
- 05: Full (1631a61a)
- 04: Full - Incremental (16432b12)
- 03: Full - Incremental (a4bcbafd)
- 02: Full - Incremental (9a735bc0)
- 01: Full - Incremental (ffdf6339)
- 00: Full (f4904691)
There is now an associated draft CSR for folks to review: https://bugs.openjdk.org/browse/JDK-8294206
@kimbarrett this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:
git checkout crt2
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push
@tschatzl did some additional performance testing and found a regression. The periodic remset sampling with associated young gen target length adjustment can interact poorly with this change. This change tries to be lazy about performing the concurrent refinement, delaying to late in the mutator phase. Because the target length adjustment happens relatively infrequently (300ms period by default), it may make a large change. But if the length is reduced by enough to make a GC needed soon, it may be too late to do much of the needed refinement. This can lead to significantly increased pause time. The solution is to move the target length adjustment into the primary refinement thread. Currently doing more perf testing.
@kimbarrett 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:
8137022: Concurrent refinement thread adjustment and (de-)activation suboptimal
8155996: Improve concurrent refinement green zone control
8134303: Introduce -XX:-G1UseConcRefinement
Reviewed-by: sjohanss, tschatzl, iwalulya, ayang
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 4 new commits pushed to the master branch:
- b37421e7578c108df87c24c93dcbc1f358f6c257: 8295564: Norwegian Nynorsk Locale is missing formatting
- 6707bfbc153de193b891c1ad3d4d8d0a6ee62307: 8029633: Raw inner class constructor ref should not perform diamond inference
- 7bc9692a5181a0db92ac2e0bca83dfe0bf2de05a: 8294670: Enhanced switch statements have an implicit default which does not complete normally
- 95dd376ba249b9eb8ab40a957238dfd79e60112f: 8291914: generated constructors are considered compact when they shouldn't
Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 master branch, type /integrate in a new comment.
Thanks @kstefanj , @tschatzl , @albertnetymk , @walulyai for reviews and additional performance testing.
Going to push as commit 028e8b3d5e7e1791a9ed0af244f74d21fb12ba81.
Since your change was applied there have been 7 commits pushed to the master branch:
- faa6b662577a24eeb726ba525303b68b87269869: 8295715: Minimize disabled warnings in serviceability libs
- de1e0c57a75efee0b171f7ad341ce8db24c5507f: 8295719: Remove unneeded disabled warnings in jdk.sctp
- 9612cf998a22a7baec7f56ba256e5d3aa3ee8c7a: 8295529: Add link to JBS to README.md
- b37421e7578c108df87c24c93dcbc1f358f6c257: 8295564: Norwegian Nynorsk Locale is missing formatting
- 6707bfbc153de193b891c1ad3d4d8d0a6ee62307: 8029633: Raw inner class constructor ref should not perform diamond inference
- 7bc9692a5181a0db92ac2e0bca83dfe0bf2de05a: 8294670: Enhanced switch statements have an implicit default which does not complete normally
- 95dd376ba249b9eb8ab40a957238dfd79e60112f: 8291914: generated constructors are considered compact when they shouldn't
Your commit was automatically rebased without conflicts.
@kimbarrett Pushed as commit 028e8b3d5e7e1791a9ed0af244f74d21fb12ba81.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.