RFR/RFC: 8220342: Remove scavenge_root_nmethods_do from VM_HeapWalkOperation::collect_simple_roots

[serguei.spitsyn at oracle.com]

Hi Stefan,

The fix looks good to me.
Testing the tiers 1-7 for different GC's has to be good enough.

Thanks,
Serguei


On 3/12/19 8:19 AM, Stefan Karlsson wrote:
> Hi all,
>
> Please review and/or comment on this change to remove 
> CodeCache::scavenge_root_nmehods_do from 
> VM_HeapWalkOperation::collect_simple_roots.
>
> http://cr.openjdk.java.net/~stefank/8220342/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8220342
>
> VM_HeapWalkOperation::collect_simple_roots is used to implement the 
> following JVMTI functionality:
>  IterateOverReachableObjects
>  IterateOverObjectsReachableFromObject
>  FollowReferences
>
> From:
> https://docs.oracle.com/en/java/javase/11/docs/specs/jvmti.html#FollowReferences 
>
>
> "This function initiates a traversal over the objects that are 
> directly and indirectly reachable from the specified object or, if 
> initial_object is not specified, all objects reachable from the heap 
> roots. The heap root are the set of system classes, JNI globals, 
> references from thread stacks, and other objects used as roots for the 
> purposes of garbage collection."
>
> The set of roots in collect_simple_roots matches this, and mostly 
> visits the set of roots that one of our class unloading enabled GCs 
> would visit.
>
> There are some roots missing. For example:
>  Management::oops_do
>  JvmtiExport::oops_do
>  AOTLoader::oops_do
>
> And there's one set of roots that is present in collect_simple_roots, 
> that is not visited by our unloading GCs:
>  CodeCache::scavenge_root_nmethods_do
>
> As an example, in PSMarkSweep we have the following comment in the 
> root scanning code:
>   // Do not treat nmethods as strong roots for mark/sweep, since we 
> can unload them.
>   //CodeCache::scavenge_root_nmethods_do(...);
>
> The CodeCache::scavenge_root_nmethods_do is only used by Serial, 
> Parallel, and CMS, to scan pointers into young gen. Other GCs don't 
> use it at all, and if we run with G1 this call does nothing at all.
>
> CodeCache::scavenge_root_nmethods_do is an GC implementation detail 
> that I want to confine with the following RFE:
>  https://bugs.openjdk.java.net/browse/JDK-8220343
>  "Move scavenge_root_nmethods from shared code"
>
> and this is the only external usage of it.
>
> Note also that the only effect of this code is that it adds a set of 
> roots that point into the young gen, but only for some of our GCs. 
> There are other roots that also point into the young gen that we don't 
> visit. For example, the non-system classes. See how 
> collect_simple_roots use ClassLoaderDataGraph::always_strong_cld_do 
> instead of ClassLoaderDataGraph::cld_do.
>
> I've run through tier1-7 with this removal, without any problems.
>
> I'd be interested in hearing if others have a justification for having 
> this code in collect_simple_roots. Or a test-case showing why this is 
> needed.
>
> There has been some brief, internal discussions that maybe we want to 
> visit all sets of roots in the vm, both strong and weak. A quick 
> implementation of that causes problem in testing when objects tagged 
> by JVMTI, and JNI weak global handles, gets reported as roots. Because 
> of that, such change requires more investigation and work than simply 
> extending the set of roots. However, if one were to go that route the 
> above call to CodeCache::scavenge_root_nmethods_do would be replaced 
> with CodeCache::blobs_do, the function used when we turn off class 
> unloading and use our weak roots as strong roots. As an example, see 
> GenCollectedHeap::process_roots:
>
>       // CMSCollector uses this to do intermediate-strength collections.
>       // We scan the entire code cache, since CodeCache::do_unloading 
> is not called.
>       CodeCache::blobs_do(code_roots);
>
> Thanks,
> StefanK