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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore

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 <[hidden email]> wrote:
>>
>> Hi Coleen,
>>
>> On 11/03/2017 12:39 AM, [hidden email] <mailto:[hidden email]> 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, [hidden email] 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Igor Veresov
Looks good. Amazing how much dead code is there.

igor


> On Apr 10, 2017, at 2:12 PM, [hidden email] 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 <[hidden email]> wrote:
>>>
>>> Hi Coleen,
>>>
>>> On 11/03/2017 12:39 AM, [hidden email] <mailto:[hidden email]> 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, [hidden email] 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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

serguei.spitsyn@oracle.com
In reply to this post by coleen.phillimore
Hi Coleen,

http://cr.openjdk.java.net/~coleenp/8026985.03/webrev/src/share/vm/classfile/dictionary.cpp.udiff.html

            // The loader in this entry is alive. If the klass is dead,
            // (determined by checking the defining class loader)
            // the loader must be an initiating loader (rather than the
            // defining loader). Remove this entry.
            if (k_def_class_loader_data->is_unloading()) {
+ ResourceMark rm;
+ tty->print_cr("loader data %s loads class %s in loader data %s",
+ loader_data->loader_name(),
+ ik->name()->as_C_string(), k_def_class_loader_data->loader_name());
+ ShouldNotReachHere(); // isn't there a dependency created? or
k_loader_data is parent of loader_data??
              // If we get here, the class_loader_data must not be the defining
              // loader, it must be an initiating one.
              assert(k_def_class_loader_data != loader_data,
                     "cannot have live defining loader and unreachable klass");
              // Loader is live, but class and its defining loader are dead.


    Not sure, I understand why is some code present after theShouldNotReachHere.     Also, one extra (unneeded?) question mark is present in the comment.

    Otherwise, the fix looks good to me.

Thanks, Serguei On 4/10/17 14:12, [hidden email] 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 <[hidden email]>
>>> wrote: Hi Coleen, On 11/03/2017 12:39 AM,
>>> [hidden email] <mailto:[hidden email]>
>>> 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, [hidden email] 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
In reply to this post by Igor Veresov
Thanks, Igor!
Coleen

On 4/10/17 8:27 PM, Igor Veresov wrote:

> Looks good. Amazing how much dead code is there.
>
> igor
>
>
>> On Apr 10, 2017, at 2:12 PM, [hidden email] 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 <[hidden email]> wrote:
>>>>
>>>> Hi Coleen,
>>>>
>>>> On 11/03/2017 12:39 AM, [hidden email] <mailto:[hidden email]> 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, [hidden email] 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
In reply to this post by serguei.spitsyn@oracle.com


On 4/11/17 3:05 AM, [hidden email] wrote:

> Hi Coleen,
>
> http://cr.openjdk.java.net/~coleenp/8026985.03/webrev/src/share/vm/classfile/dictionary.cpp.udiff.html 
>
>
>            // The loader in this entry is alive. If the klass is dead,
>            // (determined by checking the defining class loader)
>            // the loader must be an initiating loader (rather than the
>            // defining loader). Remove this entry.
>            if (k_def_class_loader_data->is_unloading()) {
> + ResourceMark rm;
> + tty->print_cr("loader data %s loads class %s in loader data %s",
> + loader_data->loader_name(),
> + ik->name()->as_C_string(), k_def_class_loader_data->loader_name());
> + ShouldNotReachHere(); // isn't there a dependency created? or
> k_loader_data is parent of loader_data??
>              // If we get here, the class_loader_data must not be the
> defining
>              // loader, it must be an initiating one.
>              assert(k_def_class_loader_data != loader_data,
>                     "cannot have live defining loader and unreachable
> klass");
>              // Loader is live, but class and its defining loader are
> dead.
>
>
>    Not sure, I understand why is some code present after
> theShouldNotReachHere.     Also, one extra (unneeded?) question mark
> is present in the comment.

Hi Serguei,  That change is part of a different investigation, I'll
remove it.  Thanks for looking at this change.

Coleen

>
>    Otherwise, the fix looks good to me.
>
> Thanks, Serguei On 4/10/17 14:12, [hidden email] 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 <[hidden email]>
>>>> wrote: Hi Coleen, On 11/03/2017 12:39 AM,
>>>> [hidden email] <mailto:[hidden email]>
>>>> 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, [hidden email] 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
In reply to this post by coleen.phillimore
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, [hidden email] 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 <[hidden email]>
>>> wrote:
>>>
>>> Hi Coleen,
>>>
>>> On 11/03/2017 12:39 AM, [hidden email]
>>> <mailto:[hidden email]> 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, [hidden email] 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
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
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, [hidden email] 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 <[hidden email]>
>>>> wrote:
>>>>
>>>> Hi Coleen,
>>>>
>>>> On 11/03/2017 12:39 AM, [hidden email]
>>>> <mailto:[hidden email]> 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, [hidden email] 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
>>

Loading...