On Wed, 3 May 2023 10:55:49 GMT, Stefan Karlsson <stef...@openjdk.org> wrote:
>> 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 develo pment 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: >> >> * 59d1e96af6a UPSTREAM: Introduce check_oop infrastructure to check oops in >> the oop class >> * ca9edf8aa79 UPSTREAM: RISCV tmp reg cleanup resolve_jobject >> * 4bec9c69b67 CLEANUP: barrierSetNMethod_aarch64.cpp >> * b67d03a3f04 UPSTREAM: Add relaxed add&fetch for aarch64 atomics >> * a2824734d23 UPSTREAM: lir_xchg >> * 36cd39c0126 UPSTREAM: assembler_ppc CMPLI >> * 447259cea42 UPSTREAM: assembler_ppc ANDI >> * 9417323499a 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. > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Fix PPC build after 8305668 test/hotspot/jtreg/ProblemList-generational-zgc.txt line 32: > 30: # Quiet all SA tests > 31: > 32: resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java 8000000 > generic-all I'd suggest filing a bug calling out the lack of SA support for generational ZGC and add a comment that there are no plans to address this. test/jdk/ProblemList-generational-zgc.txt line 27: > 25: # > 26: # List of quarantined tests for testing with Generational ZGC. > 27: # Are the tests in `test/jdk/sun/tools/jhsdb/` not failing? test/jdk/com/sun/jdi/ThreadMemoryLeakTest.java line 30: > 28: * > 29: * @comment Don't allow -Xcomp or -Xint as they impact memory useage and > number of iterations > 30: * @requires (vm.compMode == "Xmixed") & !(vm.gc.Z & > vm.opt.final.ZGenerational) Seems like a bug should be filed for this failure and then problem listed. This test is a bit finicky w.r.t. the specified max heap size and how much memory ends up actually being used by the test. I can probably get it working without much of a problem. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13771#discussion_r1184124372 PR Review Comment: https://git.openjdk.org/jdk/pull/13771#discussion_r1184126199 PR Review Comment: https://git.openjdk.org/jdk/pull/13771#discussion_r1184128793
