RFR 8026985: Rewrite SystemDictionary::classes_do and Dictionary::classes_do to use KlassClosure

[coleen.phillimore at oracle.com]

David, Thank you for reviewing.

On 3/10/17 12:18 AM, David Holmes wrote:
> Hi Coleen,
>
> On 9/03/2017 2:24 AM, coleen.phillimore at oracle.com wrote:
>> Summary: Clean up and examine uses of classes_do for the 
>> SystemDictionary
>>
>> See bug comments for more details.  I wanted to clean this up while
>> examining the idea of having system dictionary information added per
>> ClassLoaderData, rather than a global table.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8026985.01/webrev
>> bug link https://bugs.openjdk.java.net/browse/JDK-8026985
>
> The bulk of the deletion looks good! :)
>
> I guess my main query is how ClassLoaderDataGraph::classes_do / 
> loaded_classes_do relate to SystemDictionary::classes_do? I would 
> presume SD::classes_do can only act on loaded classes - by definition. 
> So then it is unclear how you can replace it with either of the CLDG 
> methods?
True, loaded_classes_do only walks loaded classes, which is probably 
what we want most of the time.  I was trying to figure this out and the 
differences which led to this change.  I am trying to make it less 
confusing.  At least for me!

The ClassLoaderData::classes_do includes anonymous, array and allocated 
classes.   Most of the time we want the first two.  For GC we want the 
last (because it has a mirror to walk).  The SystemDictionary only has 
loaded InstanceKlasses, both with defined loader and initiating 
loader.   Except for one place in the code, we ignore the entries with 
initiating loader.

The only place where methods_do() is called for all the methods is for 
print_method_data_histogram and print_method_profiling_data. These 
should only use loaded classes, so I've made the change below:

void ClassLoaderData::methods_do(void f(Method*)) {
   // Lock-free access requires load_ptr_acquire
   for (Klass* k = load_ptr_acquire(&_klasses); k != NULL; k = 
k->next_link()) {
     if (k->is_instance_klass() && InstanceKlass::cast(k)->is_loaded()) {
       InstanceKlass::cast(k)->methods_do(f);
     }
   }
}
>
> It also seems a little odd to switch from SD to CLDG for classes_do, 
> but go the other way, from CLDG to SD for methods_do ? I would 
> expect/hope to have a single "entry point" for this kind of iteration.

I know.  Unfortunately SD::methods_do includes the methods added for 
MethodHandle intrinsics, which is owned by SystemDictionary.

void SystemDictionary::methods_do(void f(Method*)) {
   ClassLoaderDataGraph::methods_do(f);
   invoke_method_table()->methods_do(f);
}

I don't like this either.   This invoke_method_table() really has 
nothing to do with the Dictionary, it's just where it ended up.  I could 
expand the change to find a better place for it.  I don't know where 
would be good though.

Aside, I'd forgotten about invoke_method_table - it seems to be a 
mechanism for isolated methods.

Coleen

>
> Thanks,
> David
>
>> Tested with java.lang.instrument, sun.com.jdi, tonga colocated (closed)
>> tests, and JPRT, because of difference in which classes_do is called for
>> heap dumping.
>>
>> Note, will update copyrights on commit.
>>
>> Thanks,
>> Coleen