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

[David Holmes]

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
>