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

[serguei.spitsyn at oracle.com]

Hi Coleen,

http://cr.openjdk.java.net/~coleenp/8026985.03/webrev/src/share/vm/classfile/dictionary.cpp.udiff.html

            // The loader in this entry is alive. If the klass is dead,
            // (determined by checking the defining class loader)
            // the loader must be an initiating loader (rather than the
            // defining loader). Remove this entry.
            if (k_def_class_loader_data->is_unloading()) {
+ ResourceMark rm;
+ tty->print_cr("loader data %s loads class %s in loader data %s",
+ loader_data->loader_name(),
+ ik->name()->as_C_string(), k_def_class_loader_data->loader_name());
+ ShouldNotReachHere(); // isn't there a dependency created? or 
k_loader_data is parent of loader_data??
              // If we get here, the class_loader_data must not be the defining
              // loader, it must be an initiating one.
              assert(k_def_class_loader_data != loader_data,
                     "cannot have live defining loader and unreachable klass");
              // Loader is live, but class and its defining loader are dead.


    Not sure, I understand why is some code present after theShouldNotReachHere.     Also, one extra (unneeded?) question mark is present in the comment.

    Otherwise, the fix looks good to me.

Thanks, Serguei On 4/10/17 14:12, coleen.phillimore at oracle.com wrote:
> Hi, I've reopened this cleanup.  To be clear, I didn't change which of 
> classes_do are called except for two places in this change. One is 
> print_method_profiling_data because it doesn't make sense for this not 
> to print methods for anonymous classes and method handle intrinsics.   
> See SystemDictionary::methods_do() implementation.   I added 
> hotspot-compiler-dev to this review so hopefully someone from the 
> compiler group can verify this. The second is heapDumper.cpp because 
> it seemed an obvious bug that if !ClassUnloading that all the classes 
> shouldn't be dumped as STICKY classes.   Tested with jvmti and 
> heapdump tests. I've put this data you requested in the bug report as 
> a comment.  We could probably file about 4 more bugs to cleanup the 
> calls sites for classes_do, so that the design is clearer, but I 
> didn't want to do it all in this one change and mix it up with the 
> functions that I found were unused and this change got large enough. 
> New webrev: open webrev at 
> http://cr.openjdk.java.net/~coleenp/8026985.03/webrev bug link 
> https://bugs.openjdk.java.net/browse/JDK-8026985 Thanks! Coleen On 
> 3/14/17 10:43 AM, Karen Kinnear wrote:
>> Coleen, I am concerned about exposure of: 1) not yet loaded classes 
>> 2) scratch-classes 3) classes that are not live due to JFR, parallel 
>> class loading or load errors Could  you please make us a table of 
>> those who walk the classes:     which classes they saw before     
>> which classes they see now - and why the change is ok     what tests 
>> you have for each of the boxes in the table - both that you see what 
>> you want, and that you do not see     what should not be exposed. My 
>> understanding is that this is intended to speed up GC but not to 
>> change behaviors. I am concerned about the exposure to non-GC 
>> walkers. thanks, Karen
>>> On Mar 13, 2017, at 8:30 PM, David Holmes <david.holmes at oracle.com> 
>>> wrote: Hi Coleen, On 11/03/2017 12:39 AM, 
>>> coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com> 
>>> wrote:
>>>> 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. 
>>> I'm still very unclear as to why it is now okay to expand the set of 
>>> klasses being walked at a given point. For example: 
>>> src/share/vm/oops/klassVtable.cpp    static void compute() { -    
>>> SystemDictionary::classes_do(do_class); +    
>>> ClassLoaderDataGraph::classes_do(do_class); IIUC SD::classes_do does 
>>> not see anonymous classes, but CLDG::classes_do does. Why is this 
>>> okay? You mentioned bugs like JDK-8024423 "missing anonymous 
>>> classes" but my limited understanding of that bug suggests to me 
>>> that the fix was to completely hide anonymous classes not to expose 
>>> them! They should not be visible to JVM TI as I understand the 
>>> intent of VM anonymous classes! Thanks, David -----
>>>> 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