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

[David Holmes]

On 15/03/2017 2:31 AM, coleen.phillimore at oracle.com wrote:
>
>
> 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.
>
> The purpose of this change was to clean out unused classes_do functions
> so that CLDG vs. SystemDictionary classes_do functions can be examined
> for this very reason.  Except for the instance that David found in
> print_vtable_statistics which was probably a bug, but I changed it back,
> I didn't change *which* of these functions was called.
>
> I'm going to withdraw this change.

Still seems reasonable to get rid of all the unused functions. It was 
the changes between SD and CLDG that were causing the confusion - and 
probably best done separately.

Thanks,
David


> We can file an RFE to examine all calls of CLDG::classes_do vs
> SystemDictionary::classes_do.
>
> thanks,
> Coleen
>
>
>
>>
>> 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
>