On Fri, 5 May 2023 07:43:17 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 with a new target base due to a > merge or a rebase. The pull request now contains 917 commits: > > - ZGC: Generational > > Co-authored-by: Stefan Karlsson <stef...@openjdk.org> > Co-authored-by: Per Liden <pli...@openjdk.org> > Co-authored-by: Albert Mingkun Yang <ay...@openjdk.org> > Co-authored-by: Erik Österlund <eosterl...@openjdk.org> > Co-authored-by: Axel Boldt-Christmas <abold...@openjdk.org> > Co-authored-by: Stefan Johansson <kstef...@openjdk.org> > - UPSTREAM: Introduce check_oop infrastructure to check oops in the oop class > - UPSTREAM: RISCV tmp reg cleanup resolve_jobject > - CLEANUP: barrierSetNMethod_aarch64.cpp > - UPSTREAM: Add relaxed add&fetch for aarch64 atomics > - UPSTREAM: assembler_ppc CMPLI > > Co-authored-by: TheRealMDoerr <martin.do...@sap.com> > - UPSTREAM: assembler_ppc ANDI > > Co-authored-by: TheRealMDoerr <martin.do...@sap.com> > - UPSTREAM: Add VMErrorCallback infrastructure > - Merge branch 'zgc_generational' into zgc_generational_rebase_target > - Whitespace nit > - ... and 907 more: https://git.openjdk.org/jdk/compare/705ad7d8...349cf9ae src/hotspot/cpu/x86/gc/z/zBarrierSetAssembler_x86.cpp line 310: > 308: // A not relocatable object could have spurious raw null pointers in > its fields after > 309: // getting promoted to the old generation. > 310: __ cmpw(ref_addr, barrier_Relocation::unpatched); `cmpw` with immediates stalls the predecoder, it may be better to `movzwl` to a spare register and `cmpl` there. src/hotspot/cpu/x86/gc/z/zBarrierSetAssembler_x86.cpp line 483: > 481: > 482: __ lock(); > 483: __ cmpxchgq(rbx, Address(rcx, 0)); `ref_addr` is not necessarily materialised here? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13771#discussion_r1186614250 PR Review Comment: https://git.openjdk.org/jdk/pull/13771#discussion_r1186640115