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

Tue Apr 11 00:27:40 UTC 2017

Looks good. Amazing how much dead code is there.

igor

> On Apr 10, 2017, at 2:12 PM, 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
> 