RFR: 8213623: ZGC: Let heap iteration walk all roots

[Kim Barrett]

> On Nov 9, 2018, at 5:28 PM, Per Liden <per.liden at oracle.com> wrote:
> 
> On 2018-11-09 21:19, Kim Barrett wrote:
>>> On Nov 9, 2018, at 9:49 AM, Per Liden <per.liden at oracle.com> wrote:
>>> 
>>> Let the ZHeapIterator walk all roots, including weak roots that might be logically cleared from the application's point of view. This allows e.g. for a more thorough heap verification.
>>> 
>>> Also the heap iterator should access oops with AS_NO_KEEPALIVE to avoid resurrecting object that would otherwise be garbage collected.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8213623
>>> Webrev: http://cr.openjdk.java.net/~pliden/8213623/webrev.0
>>> 
>>> Testing: tier1-4
>>> 
>>> /Per
>> ------------------------------------------------------------------------------
>> src/hotspot/share/gc/z/zRootsIterator.hpp
>>  127   ZConcurrentRootsIterator(bool marking = false);
>> Why is this being made optional?  It seems like there is no "safe" or
>> "obvious" default value, and that all callers should be required to
>> say what they want.
> 
> As I see it, this should only be true in one special case, which is when we're marking. Other use cases of this iterator should not have to think about saying true here.

I’ve seen enough bugs brought on by default value confusion, especially with bools, that I’m rather shy of them.
But okay.

> 
>> ------------------------------------------------------------------------------
>> src/hotspot/share/gc/z/zHeapIterator.cpp
>>  164 void ZHeapIterator::objects_do(ObjectClosure* cl) {
>>  165   // Note that the heap iterator visits all reachable objects, including
>>  166   // objects that might be unreachable from the application, such as a
>>  167   // not yet cleared JNIWeakGloablRef. However, also note that visiting
>>  168   // the JVMTI tag map is a requirement to make sure we visit all tagged
>>  169   // objects, even those that might now have become phantom reachable.
>>  170   // If we didn't do this the application would have expected to see
>>  171   // ObjectFree events for phantom reachable objects in the tag map.
>> I think there's a good rationale for walking JNIWeakGlobalRef &etc
>> here.  But I think that same rationale applies to the other
>> collectors, and I think all the collectors should be consistent about
>> this.  Making this semantic change only to ZGC seems wrong to me.
> 
> Other collectors typically walk the heap by parsing it from start to end, which means it will visit all objects, including those pointed to by e.g. JNIWeakGlobalRefs and those that are unreachable. ZGC doesn't have a parsable heap, so we instead start from the roots and visit everything we can reach from there. Semantically, this change moves ZGC a little bit closer to what other collectors do.

It seems I misunderstood how this proposed change relates to a slack
discussion we had a couple of weeks ago.

I think there was consensus from that discussion that existing heap
walkers should be using AS_NO_KEEPALIVE.  The proposed change is
addressing one case of that, which is good in so far as it goes.

ZGC implements JVMTI IterateThroughHeap via something that is similar
to but different from JVMTI's root-based FollowReferences, because the
ZGC heap is not reliably parseable in a way that would support the
kind of heap iteration done by other collectors.

Based on that earlier discussion, I think JVMTI's FollowReferences
should be including off-heap weak references like jweaks in its root
set, for reasons that are similar to what was given for including them
in ZGCs IterateThroughHeap implementation with this change.  I was
confused and thought this change was affecting FollowReferences for
ZGC.

But that leads me to question why we need the code that is being
changed here at all?  Why not fix FollowReferences, let ZGC use it as
its implementation of IterateThroughHeap, and delete some code?  Am I
missing something that makes that not work?

I think the change is okay, in that it does what it sets out to do.  I
think we might be able to do better, but I'm okay with doing so later.