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

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
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

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
|

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

David Holmes
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?

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.

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
|

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,

Nice cleanup!
It looks good to me.
I'm also interested in reply to David's questions.

One minor comment:

http://cr.openjdk.java.net/~coleenp/8026985.01/webrev/src/share/vm/services/heapDumper.cpp.udiff.html

- SystemDictionary::always_strong_classes_do(&class_dumper);
+ ClassLoaderData::the_null_class_loader_data()->classes_do(&class_dumper);
+ //SystemDictionary::always_strong_classes_do(&class_dumper); It is
probably better to remove the commented line.


Thanks,
Serguei



On 3/8/17 08:24, [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
>
> 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
|

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

coleen.phillimore
In reply to this post by David Holmes

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.

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
|

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 3/10/17 4:07 AM, [hidden email] wrote:

> Hi Coleen,
>
> Nice cleanup!
> It looks good to me.
> I'm also interested in reply to David's questions.
>
> One minor comment:
>
> http://cr.openjdk.java.net/~coleenp/8026985.01/webrev/src/share/vm/services/heapDumper.cpp.udiff.html 
>
>
> - SystemDictionary::always_strong_classes_do(&class_dumper);
> +
> ClassLoaderData::the_null_class_loader_data()->classes_do(&class_dumper);
> + //SystemDictionary::always_strong_classes_do(&class_dumper); It is
> probably better to remove the commented line.
>
>
Thank you for reviewing!    I will remove the commented line.
thanks,
Coleen

> Thanks,
> Serguei
>
>
>
> On 3/8/17 08:24, [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
>>
>> 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
|

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 3/10/17 4:07 AM, [hidden email] wrote:

> Hi Coleen,
>
> Nice cleanup!
> It looks good to me.
> I'm also interested in reply to David's questions.
>
> One minor comment:
>
> http://cr.openjdk.java.net/~coleenp/8026985.01/webrev/src/share/vm/services/heapDumper.cpp.udiff.html 
>
>
> - SystemDictionary::always_strong_classes_do(&class_dumper);
> +
> ClassLoaderData::the_null_class_loader_data()->classes_do(&class_dumper);
> + //SystemDictionary::always_strong_classes_do(&class_dumper); It is
> probably better to remove the commented line.
>

I commented this out because this was the last use of this function, so
now I've removed the function.

The placeholders->classes_do() was left over from PermGen.  It was
extracted out of oops_do() when we did that work.  I removed that also.

Thanks for noticing this.   New webrev:

open webrev at http://cr.openjdk.java.net/~coleenp/8026985.02/webrev

Thanks,
Coleen

>
> Thanks,
> Serguei
>
>
>
> On 3/8/17 08:24, [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
>>
>> 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
|

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

serguei.spitsyn@oracle.com
It looks good to me.
Thank you for the update!

Thanks,
Serguei


On 3/10/17 07:05, [hidden email] wrote:

>
>
> On 3/10/17 4:07 AM, [hidden email] wrote:
>> Hi Coleen,
>>
>> Nice cleanup!
>> It looks good to me.
>> I'm also interested in reply to David's questions.
>>
>> One minor comment:
>>
>> http://cr.openjdk.java.net/~coleenp/8026985.01/webrev/src/share/vm/services/heapDumper.cpp.udiff.html 
>>
>>
>> - SystemDictionary::always_strong_classes_do(&class_dumper);
>> +
>> ClassLoaderData::the_null_class_loader_data()->classes_do(&class_dumper);
>> + //SystemDictionary::always_strong_classes_do(&class_dumper); It is
>> probably better to remove the commented line.
>>
>
> I commented this out because this was the last use of this function,
> so now I've removed the function.
>
> The placeholders->classes_do() was left over from PermGen.  It was
> extracted out of oops_do() when we did that work.  I removed that also.
>
> Thanks for noticing this.   New webrev:
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8026985.02/webrev
>
> Thanks,
> Coleen
>>
>> Thanks,
>> Serguei
>>
>>
>>
>> On 3/8/17 08:24, [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
>>>
>>> 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
|

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

coleen.phillimore

Thanks, Serguei!
Coleen

On 3/10/17 2:58 PM, [hidden email] wrote:

> It looks good to me.
> Thank you for the update!
>
> Thanks,
> Serguei
>
>
> On 3/10/17 07:05, [hidden email] wrote:
>>
>>
>> On 3/10/17 4:07 AM, [hidden email] wrote:
>>> Hi Coleen,
>>>
>>> Nice cleanup!
>>> It looks good to me.
>>> I'm also interested in reply to David's questions.
>>>
>>> One minor comment:
>>>
>>> http://cr.openjdk.java.net/~coleenp/8026985.01/webrev/src/share/vm/services/heapDumper.cpp.udiff.html 
>>>
>>>
>>> - SystemDictionary::always_strong_classes_do(&class_dumper);
>>> +
>>> ClassLoaderData::the_null_class_loader_data()->classes_do(&class_dumper);
>>> + //SystemDictionary::always_strong_classes_do(&class_dumper); It is
>>> probably better to remove the commented line.
>>>
>>
>> I commented this out because this was the last use of this function,
>> so now I've removed the function.
>>
>> The placeholders->classes_do() was left over from PermGen. It was
>> extracted out of oops_do() when we did that work.  I removed that also.
>>
>> Thanks for noticing this.   New webrev:
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8026985.02/webrev
>>
>> Thanks,
>> Coleen
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>
>>> On 3/8/17 08:24, [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
>>>>
>>>> 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
|

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

Karen Kinnear
In reply to this post by coleen.phillimore
Coleen,

Fix email aliases.

Yes please - add this context to the bug - that will help us very much in future.
Also - thank you for adding comments to the sources.

> On Mar 10, 2017, at 2:50 PM, [hidden email] wrote:
>
>
>
> On 3/10/17 10:33 AM, Karen Kinnear wrote:
>> Coleen,
>>
>> Could you possibly write up briefly the design model here and assumptions here?
>> I don’t see them in the rfe or the sources.
>
> Hi Karen,
>
> I added this comment to classLoaderData.hpp to make it clearer:
>
>   // Walking classes through the ClassLoaderDataGraph include array classes.  It also includes
>   // classes that are allocated but not loaded, classes that have errors, and scratch classes
>   // for redefinition.  These classes are removed during the next class unloading.
>   // Walking the ClassLoaderDataGraph also includes anonymous classes.
>   static void classes_do(KlassClosure* klass_closure);
>
> There were bugs where we did not walk the ClassLoaderDataGraph and were missing anonymous classes.   This is the only one I can find right now.
> https://bugs.openjdk.java.net/browse/JDK-8024423 <https://bugs.openjdk.java.net/browse/JDK-8024423>
Many thanks for the link.
Fascinating.
The discussion with Alan Bateman  for the future “temporary classes” (name TBD) via Lookup.defineClass(…)
would make them not available via CFLH and not visible to  JVMTI getLoadedClasses or any other APIs.
Looks like that was reported internally and will require further discussion as we remove unsafe.defineAnonymousClass and move
to a cleaner model.

>
>
>>
>> It would help to really understand
>>     - who is walking classes and why
>>         - which subset they want to walk
>
> When I made this change, I visited all of the classes_do calls to verify that they were walking the class list that they want to walk.
>
> The GC versions want to walk all of the classes to mark/relocate/etc the mirrors.
Does GC want to walk scratch classes?
> Redefinition has to walk all of the classes to fix methods, actually in the function adjust_cpool_cache_and_vtable (for arrays also in case you redefine java/lang/Object which is allowed).
> HeapDumping and JFR want to show anonymous classes.

Who wants to walk the scratch classes? Is that GC only?

>
> The only exceptions were in CDS, which I didn't want to risk changing.
>
>   // Need to call SystemDictionary::classes_do(void f(Klass*, ClassLoaderData*))
>   // as we may have some classes with NULL ClassLoaderData* in the dictionary. Other
>   // variants of SystemDictionary::classes_do will skip those classes.
>   SystemDictionary::classes_do(collect_classes2);
>
> And jvmtiGetLoadedClasses, which seems to want initiating loader as well:
>
>     // First, count the classes in the system dictionary which have this loader recorded
>     // as an initiating loader. For basic type arrays this information is not recorded
>     // so GetClassLoaderClasses will return all of the basic type arrays. This is okay
>     // because the defining loader for basic type arrays is always the boot class loader
>     // and these classes are "visible" to all loaders.
>     SystemDictionary::classes_do(&JvmtiGetLoadedClassesClosure::increment_with_loader);
>
>
>
>>     - who is walking methods and why
>>          - which subset they want to walk
>>
>
> See below.  I think walking all the methods in all the classes seems dubious.  It seems better to walk the methods only in the classes that you're interested in (which is what is mostly done).  Only print_method_data_histogram and print_method_profiling_data call this for some logging.
So are those the only two callers who walk classes? Or the only ones using this methods_do?
>
>>     - when a class gets added to the CLDG, and when removed
>>
>
> A class is added to CLDG when it's allocated, ie in the allocate_instance_klass, allocate_objArrayKlass, and typeArrayKlass::allocate_klass functions.
Sorry, it was the “removed” that I wanted to make sure handled things
- errors during loading, due to JFR, due to parallel class loading
— So that those don’t get exposed or processed through any code paths walking the CLDG _klasses list.
— could you possibly check/maybe write some tests - to make sure we don’t have issues with any of the classes
that should now be unreferenced

I thought I recently reviewed code that cleaned up the find_or_define_instance_class unused class if a class was returned, i.e.
removed it from CLD - but I don’t see that in jdk10/hs right now - maybe it is not in yet. If you could keep on eye out for
that it would be helpful.

>
>> Is this a step in a bigger direction, or a one-off?
>>
>
> Good question!   To speed up class unloading, I'm looking at what it would take to split the dictionary between ClassLoaderData so that during unloading we don't have to walk dictionaries for class loaders that are on the unloaded list.   Or at least not walk them during the safepoint.  This is still an investigation but the cleanup in this RFR helps to not stumble over this code.
Do please design where to put the initiating loader information before you do the split, and think about the GC time to walk the
initiating loader list to remove entries with defining loaders that are being unloaded.
Also please ensure that the “find” time under resolve_or_null is fast - applications are very sensitive to the lookup time for
loaded classes - both success and fail times.
Thanks.

>
>> Note that we expect to add in JDK10 additional temporary classes, and will be splitting the concept of hosted vs.
>> temporary (I’m not using the term anonymous deliberately since it has java level meaning and we may have to
>> (under discussion) simultaneously support VMAC). Temporary classes would have a lifecycle which is not tied
>> to the class loader, so allow class unloading the way VMAC does today. We would remove the concept of host.
>> Some classes might belong to a nest, so they would have a nesttop, which today we are designing to allow
>> access to private members of all nest mates, i.e. all who share the same nest top.
>
> I'd like to know more about this.   The VMAC design wrt. ClassLoaderData is so that we can reclaim metaspace for their storage in fixed size chunks, which you still need with temporary classes.  But the ClassLoaderData class should be subclassed so that the temporary classes don't have to pay for the whole data structure, especially for modules and packages and anything added to support faster class unloading.
Great idea - love to hear more - it would be valuable to not have to duplicate information that is available in the primary CLD. Thanks
for thinking about that.
>
>>
>> The reason I am mentioning that is that I expect significant more use of temporary classes that are not findable, i.e.
>> not in the system dictionary.
>
> That's good.  But I think you'll need ClassLoaderDataGraph::classes_do or a version of this concept for walking to find these for GC, heap dumping, probably redefinition too if they have superclasses.
>
> Is this enough information?  I just realized that this isn't on the review thread.   Let me know if this is sufficient, and I'll cut/paste into the bug.
Added it back to the review thread - and yes - it would be great to add to the bug please.

thanks,
Karen

>
> thanks,
> Coleen
>
>>
>> thanks,
>> Karen
>>
>>> On Mar 10, 2017, at 9: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] <mailto:[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 <http://cr.openjdk.java.net/%7Ecoleenp/8026985.01/webrev>
>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8026985 <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.
>>>
>>> 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
|

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

coleen.phillimore


On 3/13/17 10:58 AM, Karen Kinnear wrote:
> Coleen,
>
> Fix email aliases.
>
> Yes please - add this context to the bug - that will help us very much
> in future.

Hi,

Ok, I've added the bulk of my reply to the bug.

> Also - thank you for adding comments to the sources.
>> On Mar 10, 2017, at 2:50 PM, [hidden email]
>> <mailto:[hidden email]> wrote:
>>
>>
>>
>> On 3/10/17 10:33 AM, Karen Kinnear wrote:
>>> Coleen,
>>>
>>> Could you possibly write up briefly the design model here and
>>> assumptions here?
>>> I don’t see them in the rfe or the sources.
>>
>> Hi Karen,
>>
>> I added this comment to classLoaderData.hpp to make it clearer:
>>
>>   // Walking classes through the ClassLoaderDataGraph include array
>> classes.  It also includes
>>   // classes that are allocated but not loaded, classes that have
>> errors, and scratch classes
>>   // for redefinition.  These classes are removed during the next
>> class unloading.
>>   // Walking the ClassLoaderDataGraph also includes anonymous classes.
>>   static void classes_do(KlassClosure* klass_closure);
>>
>> There were bugs where we did not walk the ClassLoaderDataGraph and
>> were missing anonymous classes.   This is the only one I can find
>> right now.
>> https://bugs.openjdk.java.net/browse/JDK-8024423
> Many thanks for the link.
> Fascinating.
> The discussion with Alan Bateman  for the future “temporary classes”
> (name TBD) via Lookup.defineClass(…)
> would make them not available via CFLH and not visible to  JVMTI
> getLoadedClasses or any other APIs.
> Looks like that was reported internally and will require further
> discussion as we remove unsafe.defineAnonymousClass and move
> to a cleaner model.
>>
>>
>>>
>>> It would help to really understand
>>>     - who is walking classes and why
>>>         - which subset they want to walk
>>
>> When I made this change, I visited all of the classes_do calls to
>> verify that they were walking the class list that they want to walk.
>>
>> The GC versions want to walk all of the classes to mark/relocate/etc
>> the mirrors.
> Does GC want to walk scratch classes?

Yes, they have pointers to java_mirror which is an oop, at least currently.

>> Redefinition has to walk all of the classes to fix methods, actually
>> in the function adjust_cpool_cache_and_vtable (for arrays also in
>> case you redefine java/lang/Object which is allowed).
>> HeapDumping and JFR want to show anonymous classes.
>
> Who wants to walk the scratch classes? Is that GC only?

Yes.  Although if you want to getLoadedClasses and a scratch_class is in
the list because an old method is running, I think you'd want to see that.

>>
>> The only exceptions were in CDS, which I didn't want to risk changing.
>>
>>   // Need to call SystemDictionary::classes_do(void f(Klass*,
>> ClassLoaderData*))
>>   // as we may have some classes with NULL ClassLoaderData* in the
>> dictionary. Other
>>   // variants of SystemDictionary::classes_do will skip those classes.
>>   SystemDictionary::classes_do(collect_classes2);
>>
>> And jvmtiGetLoadedClasses, which seems to want initiating loader as well:
>>
>>     // First, count the classes in the system dictionary which have
>> this loader recorded
>>     // as an initiating loader. For basic type arrays this
>> information is not recorded
>>     // so GetClassLoaderClasses will return all of the basic type
>> arrays. This is okay
>>     // because the defining loader for basic type arrays is always
>> the boot class loader
>>     // and these classes are "visible" to all loaders.
>> SystemDictionary::classes_do(&JvmtiGetLoadedClassesClosure::increment_with_loader);
>>
>>
>>
>>>     - who is walking methods and why
>>>          - which subset they want to walk
>>>
>>
>> See below.  I think walking all the methods in all the classes seems
>> dubious.  It seems better to walk the methods only in the classes
>> that you're interested in (which is what is mostly done).  Only
>> print_method_data_histogram and print_method_profiling_data call this
>> for some logging.
> So are those the only two callers who walk classes? Or the only ones
> using this methods_do?

Yes.

>>
>>>     - when a class gets added to the CLDG, and when removed
>>>
>>
>> A class is added to CLDG when it's allocated, ie in the
>> allocate_instance_klass, allocate_objArrayKlass, and
>> typeArrayKlass::allocate_klass functions.
> Sorry, it was the “removed” that I wanted to make sure handled things
> - errors during loading, due to JFR, due to parallel class loading
> — So that those don’t get exposed or processed through any code paths
> walking the CLDG _klasses list.
> — could you possibly check/maybe write some tests - to make sure we
> don’t have issues with any of the classes
> that should now be unreferenced

Klasses are only removed during a safepoint because the GCs (CMS and G1)
can walk the CLD::_klasses list concurrently.

The partially loaded/error classes *will* be processed through the CLDG
list, but this is where we added loaded_classes_do() for the bug I
pointed to earlier: https://bugs.openjdk.java.net/browse/JDK-8024423 
changeset http://hg.openjdk.java.net/jdk9/dev/hotspot/rev/e64f1fe9756b 
There were tests that failed because of this bug.

Going through the source code to examine calls to
ClassLoaderDataGraph::classes_do, the calls in redefinition need to walk
scratch_class, and I don't think it would be observable if they also
walk the not-yet loaded classes.   If heap dumper walks unloaded
classes, I don't think there would be any way to observe this behavior
since it mostly reports on the mirror.  The GC code has to walk all the
classes.

>
> I thought I recently reviewed code that cleaned up the
> find_or_define_instance_class unused class if a class was returned, i.e.
> removed it from CLD - but I don’t see that in jdk10/hs right now -
> maybe it is not in yet. If you could keep on eye out for
> that it would be helpful.
>

Yes, it's been checked in.

>>
>>> Is this a step in a bigger direction, or a one-off?
>>>
>>
>> Good question!   To speed up class unloading, I'm looking at what it
>> would take to split the dictionary between ClassLoaderData so that
>> during unloading we don't have to walk dictionaries for class loaders
>> that are on the unloaded list.   Or at least not walk them during the
>> safepoint.  This is still an investigation but the cleanup in this
>> RFR helps to not stumble over this code.
> Do please design where to put the initiating loader information before
> you do the split, and think about the GC time to walk the
> initiating loader list to remove entries with defining loaders that
> are being unloaded.

The idea is to have the DictionaryEntries be the same thing as they are
today, minus the _loader_data, since that's the CLD owner, so they would
include classes for both this CLD being the initiating and defining
class loader.  Possibly making CLD::_klasses redundant.  Still trying to
walk through all the relevant code (including the refcounted
ProtectionDomainTable) and see what's possible without changing too much :)

> Also please ensure that the “find” time under resolve_or_null is fast
> - applications are very sensitive to the lookup time for
> loaded classes - both success and fail times.

Yes, agreed!

> Thanks.
>
>>
>>> Note that we expect to add in JDK10 additional temporary classes,
>>> and will be splitting the concept of hosted vs.
>>> temporary (I’m not using the term anonymous deliberately since it
>>> has java level meaning and we may have to
>>> (under discussion) simultaneously support VMAC). Temporary classes
>>> would have a lifecycle which is not tied
>>> to the class loader, so allow class unloading the way VMAC does
>>> today. We would remove the concept of host.
>>> Some classes might belong to a nest, so they would have a nesttop,
>>> which today we are designing to allow
>>> access to private members of all nest mates, i.e. all who share the
>>> same nest top.
>>
>> I'd like to know more about this.   The VMAC design wrt.
>> ClassLoaderData is so that we can reclaim metaspace for their storage
>> in fixed size chunks, which you still need with temporary classes.  
>> But the ClassLoaderData class should be subclassed so that the
>> temporary classes don't have to pay for the whole data structure,
>> especially for modules and packages and anything added to support
>> faster class unloading.
> Great idea - love to hear more - it would be valuable to not have to
> duplicate information that is available in the primary CLD. Thanks
> for thinking about that.

Right, or have the temporary classes have degenerate CLDs and be on the
graph so they can be walked when needed.  Having all classes walked from
one place like CLD and filtering for the ones you want, puts the special
case code in one place where it's easier to deal with.

>>
>>>
>>> The reason I am mentioning that is that I expect significant more
>>> use of temporary classes that are not findable, i.e.
>>> not in the system dictionary.
>>
>> That's good.  But I think you'll need
>> ClassLoaderDataGraph::classes_do or a version of this concept for
>> walking to find these for GC, heap dumping, probably redefinition too
>> if they have superclasses.
>>
>> Is this enough information?  I just realized that this isn't on the
>> review thread.   Let me know if this is sufficient, and I'll
>> cut/paste into the bug.
> Added it back to the review thread - and yes - it would be great to
> add to the bug please.

Thanks for the comments and looking forward to more discussion.

Coleen

>
> thanks,
> Karen
>>
>> thanks,
>> Coleen
>>
>>>
>>> thanks,
>>> Karen
>>>
>>>> On Mar 10, 2017, at 9: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]
>>>>> <mailto:[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 
>>>>>> <http://cr.openjdk.java.net/%7Ecoleenp/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.
>>>>
>>>> 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
|

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,

On 11/03/2017 12:39 AM, [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
|

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

Karen Kinnear
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
|

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

coleen.phillimore
In reply to this post by David Holmes


On 3/13/17 8:30 PM, David Holmes wrote:

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

In the code it is very unclear, true.   This change was mostly a cleanup
of unused functions so that I could find and examine which of CLDG vs.
SystemDictionary classes_do was used and why.   Maybe I oversold this
change as making this not confusing.   As of now, it's ad-hoc which one
you call.

>
> 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?
>

This function is reporting sizes of vtables and bytes used for vtables.  
Since array and anonymous classes also have vtables, it looked like a
bug that they weren't reported.  I'll change it back to
SystemDictionary::classes_do.   I'm not sure what the purpose of this
print_vtable_statistics is.


> 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!

The problem was that anonymous classes weren't reported so weren't
marked as loaded by jvmti (?)   It is true that CFLH does not apply to
anonymous classes but that fix is in classFileParser.

Coleen

>
> 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
|

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

coleen.phillimore
In reply to this post by Karen Kinnear


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.

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 <[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
|

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

David Holmes
On 15/03/2017 2:31 AM, [hidden email] 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 <[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
|

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

Karen Kinnear
In reply to this post by coleen.phillimore

> On Mar 14, 2017, at 11:42 AM, [hidden email] <mailto:[hidden email]> wrote:
>
>
>
> On 3/13/17 8:30 PM, David Holmes 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] <mailto:[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 <http://cr.openjdk.java.net/~coleenp/8026985.01/webrev>
>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8026985 <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:
>
> In the code it is very unclear, true.   This change was mostly a cleanup of unused functions so that I could find and examine which of CLDG vs. SystemDictionary classes_do was used and why.   Maybe I oversold this change as making this not confusing.   As of now, it's ad-hoc which one you call.
>>
>> 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?
>>
>
> This function is reporting sizes of vtables and bytes used for vtables.  Since array and anonymous classes also have vtables, it looked like a bug that they weren't reported.  I'll change it back to SystemDictionary::classes_do.   I'm not sure what the purpose of this print_vtable_statistics is.
>
>
>> 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!
>
> The problem was that anonymous classes weren't reported so weren't marked as loaded by jvmti (?)   It is true that CFLH does not apply to anonymous classes but that fix is in classFileParser.
>
We should have a conversation before changing that. I believe going forward that the intention is that temporary/anonymous classes would not
be marked as loaded by jvmti and CFLH would not be intended to apply to them.

thanks,
Karen

> Coleen
>>
>> 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
|

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

coleen.phillimore
In reply to this post by Karen Kinnear

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
|

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
|

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
|

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

12