RFR 8026985: Rewrite SystemDictionary::classes_do and Dictionary::classes_do to use KlassClosure
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Mar 13 21:15:09 UTC 2017
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, coleen.phillimore at oracle.com
>> <mailto: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
> 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, 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
>>>>>
>>>>> 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