Skip to content
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

8307058: Implementation of Generational ZGC #13771

Closed
wants to merge 932 commits into from

Conversation

stefank
Copy link
Member

@stefank stefank commented May 3, 2023

Hi all,

Please review the implementation of Generational ZGC, which can be turned on by adding -XX:+ZGenerational in addition to using -XX:+UseZGC. Generational ZGC is a major rewrite of the non-generational ZGC version that exists in the openjdk/jdk repository. It splits the heap into two generations; the young generation where newly allocated objects are born, and the old generation where long-lived objects get promoted to. The motivation for introducing generations is to allow ZGC to reclaim memory faster by not having to walk the entire object graph every time a garbage collection is run. This should make Generational ZGC suitable for more workloads. In particular workloads that previously hit allocation stalls because of high allocation rates, large live sets, or limited spare machine resources, have the potential to work better with Generational ZGC. For an in-depth description of Generational ZGC, see https://openjdk.org/jeps/439.

The development of Generational ZGC started around the same time as the development of JDK 17. At that point we forked off the Generational ZGC development into its own branch and let non-generational live unaffected in openjdk/jdk. This safe-guarded non-generational ZGC and allowed Generational ZGC to move unhindered, without the shackles of having to fit into another GC implementation's design and quirks. Since then, almost all of the ZGC files have been changed. Moving forward to today, when it's ready for us to upstream Generational ZGC, we now need to deliver Generational ZGC without disrupting our current user-base. We have therefore opted to initially include both versions of ZGC in the code base, but with the intention to deprecate non-generational ZGC in a future release. Existing users running with only -XX:+UseZGC will get the non-generational ZGC, and users that want the new Generational ZGC need to run with -XX:+ZGenerational in addition to -XX:+UseZGC. The intention is to give the users time to validate and deploy their workloads with the new GC implementation.

Including both the new evolution of a GC and its legacy predecessor poses a few challenges for us GC developers. The first reaction could be to try to mash the two implementations together and sprinkle the GC code with conditional statements or dynamic dispatches. We have done similar experiments before. When ZGC was first born, we started an experiment where we converted G1 into getting the same features as the evolving ZGC. It was quite clear to us how time consuming and complex things end up being when we tried to keep both the original G1 working, and at the same time implemented the ZGC-alike G1. Given this experience, we don't see that as a viable solution to deliver a maintainable and evolving Generational ZGC. Our pragmatic suggestion to these challenges is to let Generational ZGC live under the current gc/z directories and let the legacy, non-generational ZGC be completely separated in its own directories. This way we can continue to move quickly with the continued development of Generational ZGC and let the non-generational ZGC be mostly untouched until it gets deprecated, and eventually removed. The non-generational ZGC directory will be gc/x and all the classes of non-generational have been prefixed with X instead of Z. An alternative to this rename could be to namespace out non-generational ZGC. We experimented with that, but it was too easy to accidentally cross-compile Generational ZGC code into non-generational ZGC, so we didn't like that approach.

Most of the stand-alone cleanups and enhancements outside of the ZGC code have already been upstreamed to openjdk/jdk. There are still a few patches that could/should be pushed separately, but they will be easier to understand by also looking at the Generational ZGC code, so they will be sent out after this PR has been published. The patches that could be published separately are:

  • 59d1e96 UPSTREAM: Introduce check_oop infrastructure to check oops in the oop class
  • ca9edf8 UPSTREAM: RISCV tmp reg cleanup resolve_jobject
  • 4bec9c6 CLEANUP: barrierSetNMethod_aarch64.cpp
  • b67d03a UPSTREAM: Add relaxed add&fetch for aarch64 atomics
  • a282473 UPSTREAM: lir_xchg
  • 36cd39c UPSTREAM: assembler_ppc CMPLI
  • 447259c UPSTREAM: assembler_ppc ANDI
  • 9417323 UPSTREAM: Add VMErrorCallback infrastructure

Regarding all the changesets you see in this PR, they form the history of the development of Generational ZGC. It might look a bit unconventional to what you are used to see in openjdk development. What we have done is to use merges with the 'ours' strategy to ignore the previous Generational ZGC patches, and then rebased and flattened the changes on top of the merge. This effectively gives us the upsides of having a rebased repository and the upsides of retaining the history in the repository. The downside could be that GitHub now lists all those changesets in the PR. Given that this patch is so big, and that you likely only want to see a part of it, I suggest that you pull down the PR branch and then compare it to the openjdk/jdk changeset this PR is based against:

git fetch https://github.com/openjdk/zgc zgc_master
git diff zgc_master... <sub-directory of interest>

There have been many contributors of this patch over the years. I'll do my best to poke Skara into listing you all, but if you see that I've missed your name please reach out to me and I'll fix it.

Testing: we have been continuously running Generational ZGC through Oracle's tier1-8 testing.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires CSR request JDK-8307059 to be approved

Issues

Reviewers

Contributors

  • Stefan Karlsson <stefank@openjdk.org>
  • Erik Österlund <eosterlund@openjdk.org>
  • Axel Boldt-Christmas <aboldtch@openjdk.org>
  • Per Liden <pliden@openjdk.org>
  • Stefan Johansson <sjohanss@openjdk.org>
  • Albert Mingkun Yang <ayang@openjdk.org>
  • Erik Helin <ehelin@openjdk.org>
  • Roberto Castañeda Lozano <rcastanedalo@openjdk.org>
  • Nils Eliasson <neliasso@openjdk.org>
  • Martin Doerr <mdoerr@openjdk.org>
  • Leslie Zhai <lzhai@openjdk.org>
  • Fei Yang <fyang@openjdk.org>
  • Yadong Wang <yadongwang@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13771/head:pull/13771
$ git checkout pull/13771

Update a local copy of the PR:
$ git checkout pull/13771
$ git pull https://git.openjdk.org/jdk.git pull/13771/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13771

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13771.diff

Webrev

Link to Webrev Comment

stefank and others added 30 commits February 9, 2023 12:22
Co-authored-by: Erik Österlund <eosterlund@openjdk.org>
Co-authored-by: Erik Österlund <eosterlund@openjdk.org>
Co-authored-by: Yadong Wang <yadongwang@openjdk.org>
Co-authored-by: Erik Österlund <eosterlund@openjdk.org>
Co-authored-by: Erik Österlund <eosterlund@openjdk.org>
Co-authored-by: Yadong Wang <yadongwang@openjdk.org>
Co-authored-by: TheRealMDoerr <martin.doerr@sap.com>
Co-authored-by: TheRealMDoerr <martin.doerr@sap.com>
Co-authored-by: Stefan Karlsson <stefank@openjdk.org>
Co-authored-by: Stefan Karlsson <stefank@openjdk.org>
Co-authored-by: Per Liden <pliden@openjdk.org>
Co-authored-by: Albert Mingkun Yang <ayang@openjdk.org>
Co-authored-by: Erik Österlund <eosterlund@openjdk.org>
Co-authored-by: Axel Boldt-Christmas <aboldtch@openjdk.org>
Co-authored-by: Stefan Johansson <kstefanj@openjdk.org>
Co-authored-by: Erik Österlund <eosterlund@openjdk.org>
Co-authored-by: Erik Österlund <eosterlund@openjdk.org>
Co-authored-by: Yadong Wang <yadongwang@openjdk.org>
Co-authored-by: TheRealMDoerr <martin.doerr@sap.com>
Co-authored-by: TheRealMDoerr <martin.doerr@sap.com>
Co-authored-by: Stefan Karlsson <stefank@openjdk.org>
xmas92 and others added 3 commits May 8, 2023 12:15
Co-authored-by: Stefan Karlsson <stefank@openjdk.org>
Co-authored-by: Per Liden <pliden@openjdk.org>
Co-authored-by: Albert Mingkun Yang <ayang@openjdk.org>
Co-authored-by: Erik Österlund <eosterlund@openjdk.org>
Co-authored-by: Axel Boldt-Christmas <aboldtch@openjdk.org>
Co-authored-by: Stefan Johansson <kstefanj@openjdk.org>
@openjdk
Copy link

openjdk bot commented May 8, 2023

@stefank 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:

8307058: Implementation of Generational ZGC

Co-authored-by: Stefan Karlsson <stefank@openjdk.org>
Co-authored-by: Erik Österlund <eosterlund@openjdk.org>
Co-authored-by: Axel Boldt-Christmas <aboldtch@openjdk.org>
Co-authored-by: Per Liden <pliden@openjdk.org>
Co-authored-by: Stefan Johansson <sjohanss@openjdk.org>
Co-authored-by: Albert Mingkun Yang <ayang@openjdk.org>
Co-authored-by: Erik Helin <ehelin@openjdk.org>
Co-authored-by: Roberto Castañeda Lozano <rcastanedalo@openjdk.org>
Co-authored-by: Nils Eliasson <neliasso@openjdk.org>
Co-authored-by: Martin Doerr <mdoerr@openjdk.org>
Co-authored-by: Leslie Zhai <lzhai@openjdk.org>
Co-authored-by: Fei Yang <fyang@openjdk.org>
Co-authored-by: Yadong Wang <yadongwang@openjdk.org>
Reviewed-by: eosterlund, aboldtch, rcastanedalo

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 master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels May 8, 2023
@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels May 9, 2023
xmas92 and others added 6 commits May 9, 2023 09:14
Co-authored-by: TheRealMDoerr <martin.doerr@sap.com>
Co-authored-by: TheRealMDoerr <martin.doerr@sap.com>
Co-authored-by: Stefan Karlsson <stefank@openjdk.org>
Co-authored-by: Per Liden <pliden@openjdk.org>
Co-authored-by: Albert Mingkun Yang <ayang@openjdk.org>
Co-authored-by: Erik Österlund <eosterlund@openjdk.org>
Co-authored-by: Axel Boldt-Christmas <aboldtch@openjdk.org>
Co-authored-by: Stefan Johansson <kstefanj@openjdk.org>
@openjdk openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels May 9, 2023
@stefank
Copy link
Member Author

stefank commented May 9, 2023

FYI: GitHub doesn't seem to handle our new merges correctly and lists changes that we have recently merged in from openjdk/jdk. Make sure to look at the patches locally or use the webrevs above.

Copy link
Member

@xmas92 xmas92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having been able to contribute to generational ZGC over the last year has been a privilege.

The code is well structured and the preparatory upstreaming work has made the integration with other subcomponents of hotspot rather minimal.

The X/Z split feels like a pragmatic solution for having the two ZGC codebases coexisting.

Just like Erik, I am a little biased. I approve of this PR!

Copy link
Contributor

@robcasloz robcasloz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiler-related changes look good!

@stefank
Copy link
Member Author

stefank commented May 11, 2023

Thanks all who have helped building Generational ZGC! I think it is time to integrate.
/integrate

@openjdk
Copy link

openjdk bot commented May 11, 2023

Going to push as commit d20034b.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 11, 2023
@openjdk openjdk bot closed this May 11, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 11, 2023
@openjdk
Copy link

openjdk bot commented May 11, 2023

@stefank Pushed as commit d20034b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
Development

Successfully merging this pull request may close these issues.