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

Tue Apr 11 14:00:00 UTC 2017

On 4/11/17 3:05 AM, serguei.spitsyn at oracle.com wrote:
> 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.

Hi Serguei,  That change is part of a different investigation, I'll 
remove it.  Thanks for looking at this change.

Coleen

>
>    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 