[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR 8026985: Rewrite SystemDictionary::classes_do and Dictionary::classes_do to use KlassClosure
From: coleen.phillimore () oracle ! com
Date: 2017-04-12 11:59:03
Message-ID: 46666df9-b107-ccd7-05d9-cdf1d1b0d2db () oracle ! com
[Download RAW message or body]
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@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@oracle.com>
>>>> wrote:
>>>>
>>>> Hi Coleen,
>>>>
>>>> On 11/03/2017 12:39 AM, coleen.phillimore@oracle.com
>>>> <mailto:coleen.phillimore@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@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
>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic