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

[Stefan Karlsson]

Thanks, Serguei!

StefanK

On 2019-03-12 22:50, serguei.spitsyn at oracle.com wrote:
> 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
>