RFR 8026985: Rewrite SystemDictionary::classes_do and Dictionary::classes_do to use KlassClosure
Karen Kinnear
karen.kinnear at oracle.com
Mon Mar 13 14:58:59 UTC 2017
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, coleen.phillimore at oracle.com 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, coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com> wrote:
>>>
>>>
>>> David, Thank you for reviewing.
>>>
>>> On 3/10/17 12:18 AM, David Holmes wrote:
>>>> Hi Coleen,
>>>>
>>>> On 9/03/2017 2:24 AM, coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com> wrote:
>>>>> Summary: Clean up and examine uses of classes_do for the SystemDictionary
>>>>>
>>>>> See bug comments for more details. I wanted to clean this up while
>>>>> examining the idea of having system dictionary information added per
>>>>> ClassLoaderData, rather than a global table.
>>>>>
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8026985.01/webrev <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
>>
>
More information about the hotspot-runtime-dev
mailing list