8255471: ZGC: Rework root iterators and closures

[stefank]

Today we have the ZRootsIteratorClosure type to cater for the different type of roots and their containers (oop storage, threads, nmethods, runtime oop data structures). This closure is then passed down and applied to the different containers. We split the root sets into different sets: concurrent strong, concurrent weak, and non-concurrent weak. Not all of those sets contain all types of containers. This forces some of the code to implement, or at least consider, parts of the ZRootsIteratorClosure interface that it doesn't care about. It also moves some of the closure logic into shared code, even when it's only used by one of the closures.

I propose that we turn this inside out, and use direct closures that match the visited containers (NMethodClosure for nmethods, et.c.). The intention is to make it a bit easier to see from the call sites what code is run and, hopefully, making localized changes stay more local in the code.

Note: I started this code with a version building upon https://bugs.openjdk.java.net/browse/JDK-8212879, but I don't want to wait for that work to finish before sending this out, so the current patch contains handling of non-conurrent weak roots. As soon as that gets integrated, we can get rid of the ZWeakSerial and the associated non-concurrent weak roots handling.

I've tested this on tier1-7 on linux x64.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Testing

	Linux x64	Linux x86	Windows x64	macOS x64
Build	✔️ (5/5 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)	✔️ (9/9 passed)

Issue

• JDK-8255471: ZGC: Rework root iterators and closures

Reviewers

• Erik Österlund (@fisk - Reviewer)
• Per Liden (@pliden - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/928/head:pull/928
$ git checkout pull/928

[stefank]

/label add hotspot-gc

[bridgekeeper]

👋 Welcome back stefank! 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.

[openjdk]

@stefank
The hotspot-gc label was successfully added.

[mlbridge]

Webrevs

• 03: Full - Incremental (caae91a)
• 02: Full - Incremental (431060b)
• 01: Full - Incremental (f4cd99a)
• 00: Full (1d6829b)

[fisk]

In the patch, the heap iterator uses a ThreadToOopClosure, which implicitly uses a CodeBlobToOop closure for on-stack nmethods. I don't know how I feel about this. I think it might be desirable to use a ZThreadToOopClosure instead, that you pass in the explicitly passed in NMethodClosure to instead. Or something along those lines, the important thing being to pass in the NMethodClosure passed to the iterator, and not use a CodeBlobToOopClosure instead. Otherwise, it is rather confusing if you explicitly pass in an NMethodClosure to the strong root iterator, telling what should be done to strong nmethods, but then it is not applied to nmethods encountered on the stack. Instead, it is only applied to the code cache nmethods, when ClassUnloading is disabled. However, there does not seem to be any reason for distinguishing the two cases, AFAICT. The NMethodClosure describes what should be done to a strong nmethod, and how we found the strong nmethod (stack or code cache depending on ClassUnloading), does not seem to make a difference. Differenting the two does however conflict with the goal of making the code more straight and understandable. If you pass in an NMethodClosure to the strong root iterator, I would expect that NMethodClosure to be applied to all strong nmethods found, and not have to deal with what happens with and without class unloading as a special case, that does not look like it would have to be special.

On a different yet related note, I see that ZHeapIteratorRootOopClosure now always performs some kind of NativeAccess::oop_load on all oops, including misaligned nmethod oops. I'm not sure how I feel about that. I think it's fine, but let me elaborate my thoughts. I suppose the contract for how it is used is that the oops have already been fixed by nmethod barriers (under the per-nmethod lock) to be good, before loading potentially misaligned oops. That should imply that the oops are never self healed, which is a requirement to not cause undefined behaviour on the HW level, performing atomics on misaligned data. So I suppose ensuring the nmethod barrier has completed before reading is crucial here. When reading the NMethodClosure, it is clear how that is the case. We apply the nmethod barrier and then walk the oops. But using the CodeBlobToOopClosure, now with parallel iteration as of lately, does seem a bit scary. I guess it still is okay, because Thread::oops_do() calls finish_processing(), which ensures all on-stack nmethods enter their barriers before applying the OopClosure. So I guess this all works correctly, but again would read a bit more intuitively for me at least, if we had a ZThreadToOopClosure that you pass in the root iterator supplied NMethodClosure to. I think it works with and without that, but would be easier to understand why it works if the NMethodClosure was passed in to the [Z?]ThreadToOopClosure. Again, it might just be me thinking that would be easier to understand, but I would at least like to point that out, so I can hear your thoughts about it.

Apart from that, this looks great, and I think it is much more readable now that you can just tell the iterators what to do with various types of roots, without getting random callbacks that may or may not make sense. Great job!

[stefank]

In the patch, the heap iterator uses a ThreadToOopClosure, which implicitly uses a CodeBlobToOop closure for on-stack nmethods. I don't know how I feel about this. I think it might be desirable to use a ZThreadToOopClosure instead, that you pass in the explicitly passed in NMethodClosure to instead. Or something along those lines, the important thing being to pass in the NMethodClosure passed to the iterator, and not use a CodeBlobToOopClosure instead. Otherwise, it is rather confusing if you explicitly pass in an NMethodClosure to the strong root iterator, telling what should be done to strong nmethods, but then it is not applied to nmethods encountered on the stack. Instead, it is only applied to the code cache nmethods, when ClassUnloading is disabled. However, there does not seem to be any reason for distinguishing the two cases, AFAICT. The NMethodClosure describes what should be done to a strong nmethod, and how we found the strong nmethod (stack or code cache depending on ClassUnloading), does not seem to make a difference. Differenting the two does however conflict with the goal of making the code more straight and understandable. If you pass in an NMethodClosure to the strong root iterator, I would expect that NMethodClosure to be applied to all strong nmethods found, and not have to deal with what happens with and without class unloading as a special case, that does not look like it would have to be special.

This part was a preexisiting "issue" that simply kept. Talked to Erik and I'll change this in a follow-up RFE.

On a different yet related note, I see that ZHeapIteratorRootOopClosure now always performs some kind of NativeAccess::oop_load on all oops, including misaligned nmethod oops. I'm not sure how I feel about that. I think it's fine, but let me elaborate my thoughts. I suppose the contract for how it is used is that the oops have already been fixed by nmethod barriers (under the per-nmethod lock) to be good, before loading potentially misaligned oops. That should imply that the oops are never self healed, which is a requirement to not cause undefined behaviour on the HW level, performing atomics on misaligned data. So I suppose ensuring the nmethod barrier has completed before reading is crucial here. When reading the NMethodClosure, it is clear how that is the case. We apply the nmethod barrier and then walk the oops. But using the CodeBlobToOopClosure, now with parallel iteration as of lately, does seem a bit scary. I guess it still is okay, because Thread::oops_do() calls finish_processing(), which ensures all on-stack nmethods enter their barriers before applying the OopClosure. So I guess this all works correctly, but again would read a bit more intuitively for me at least, if we had a ZThreadToOopClosure that you pass in the root iterator supplied NMethodClosure to. I think it works with and without that, but would be easier to understand why it works if the NMethodClosure was passed in to the [Z?]ThreadToOopClosure. Again, it might just be me thinking that would be easier to understand, but I would at least like to point that out, so I can hear your thoughts about it.

I didn't change this either, but I did drop the verification that showed that the nmethods had been handled by the stack watermark processing. I've pushed an update that reintroduces the verification. I'll fix the pre-existing problem as a separate RFE.

Apart from that, this looks great, and I think it is much more readable now that you can just tell the iterators what to do with various types of roots, without getting random callbacks that may or may not make sense. Great job!

Thanks Erik.

[fisk]

Looks good.

[openjdk]

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

8255471: ZGC: Rework root iterators and closures

Reviewed-by: eosterlund, pliden

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 78 new commits pushed to the master branch:

• 120aec7: 8255720: Optimize bci_to_dp/-data by enabling iteration over raw DataLayouts
• 4b775e6: 8255721: Remove no-op clean_weak_method_links methods
• 3302d3a: 8255544: Create a checked cast
• 54c8813: 8255734: VM should ignore SIGXFSZ on ppc64, s390 too
• ceab9f3: 6816284: Notepad class should be public
• eb66418: 7124397: [macosx] JSpinner serialiazation - deserialization issue
• 79a010f: 8255697: LogTargetHandle::print should check if log level is enabled
• 98c91b6: 8233637: [TESTBUG] Swing ActionListenerCalledTwiceTest.java fails on macos
• e97809d: 8233641: [TESTBUG] JMenuItem test bug4171437.java fails on macos
• 69f5235: 8255596: Mutex safepoint checking options and flags should be scoped enums
• ... and 68 more: https://git.openjdk.java.net/jdk/compare/3f8bd9230892a58ab8016eb746c9281d441ffe1c...master

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.

[stefank]

Thanks @fisk

[stefank]

Thanks @pliden

/integrate

[openjdk]

@stefank Since your change was applied there have been 82 commits pushed to the master branch:

• b028074: 8255673: Wrong version in docs bundles
• 4c66b15: 8255235: ZGC: Allocate and initialize forwarding data structures in parallel
• 1019581: 8255691: Shenandoah: Invoke native-LRB only on non-strong refs
• d2812f7: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build
• 120aec7: 8255720: Optimize bci_to_dp/-data by enabling iteration over raw DataLayouts
• 4b775e6: 8255721: Remove no-op clean_weak_method_links methods
• 3302d3a: 8255544: Create a checked cast
• 54c8813: 8255734: VM should ignore SIGXFSZ on ppc64, s390 too
• ceab9f3: 6816284: Notepad class should be public
• eb66418: 7124397: [macosx] JSpinner serialiazation - deserialization issue
• ... and 72 more: https://git.openjdk.java.net/jdk/compare/3f8bd9230892a58ab8016eb746c9281d441ffe1c...master

Your commit was automatically rebased without conflicts.

Pushed as commit 1769c48.

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