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

Wed Apr 12 11:59:03 UTC 2017

Thank you, David!
Coleen

On 4/12/17 3:26 AM, David Holmes wrote:
> Hi Coleen,
>
> This all seems quite reasonable. It is good to see so much code deleted!
>
> Thanks,
> David
>
> On 11/04/2017 7:12 AM, 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
>>