RFR (S) JDK-8007725 - Klass::restore_unshareable_info() triggers assert(k->java_mirror() == NULL)
Ioi Lam
ioi.lam at oracle.com
Thu Mar 14 14:01:00 PDT 2013
Thanks to everyone for the reviews. I have updated my patch:
http://cr.openjdk.java.net/~iklam/8007725/null_mirror_002/
Changes are:
* Changed the clean up code according to Karen's feedbacks
* Removed obsolete comment in systemDictionary.cpp related to
MarkAndMoveOrderedReadWrite
* Added more comments inside method.cpp, and modified the assert
according to Harold.
Thanks
- Ioi
On 03/14/2013 01:34 PM, Karen Kinnear wrote:
> Thank you Ioi,
>
> more below
>>>>> 5. systemDictionary.cpp
>>>>>
>>>>> I'm wondering if you want to move the cleanup request to a different place.
>>>>>
>>>>> If the load was successful, but the check_constraints on the initiating loader fails - I think you want to
>>>>> leave the klass loaded by the defining loader (null) and leave the information intact.
>>>>> (We allow a defineclass to succeed and future lookups to find the class/defining loader even if
>>>>> the initiating loader fails due to loader constraints or later checkpackageaccess for instance)
>>>>>
>>>>> I don't quite understand where the exception is happening that you are cleaning up for.
>>>>> Where did you see an OOM? Where else could they happen here? What other
>>>>> exceptions could happen? I think this has to be handled at a tighter level of granularity so
>>>>> we know exactly when this clean up is needed.
>>>> In testing, I saw OOM inside Klass::restore_unshareable_info() => java_lang_Class::create_mirror().
>>>>
>>>> But InstanceKlass::restore_unshareable_info() actually makes a lot of allocations, so it could fail at a number of different places.
>>> Thank you. So one of the locations that could cause OOM problems would be restore_unshareable_info. Thanks.
>>> And there is the possibility of getting a variety of exceptions from the find_or_define path.
>>>> The main reason for the clean up is this:
>>>>
>>>> In SystemDictionary::resolve_instance_class_or_null, if the target class is not in the system dictionary, then load_instance_class() will be called, which eventually will unconditionally call into InstanceKlass::restore_unshareable_info().
>>> (I think that is conditional on find_shared_class, and the resolve_super for superclasses/superinterfaces succeeding - those have CHECKs so if they fail, we don't get here)
>>>> So the clean up should be done whenever we have failed to put the shared class into the dictionary.
>>> I agree.
>>>> I am thinking of leaving the clean up at the same place, but do this instead:
>>>>
>>>> ....
>>>> } // load_instance_class loop
>>>>
>>>> if (HAS_PENDING_EXCEPTION) {
>>>> // An OOM could have happened at various places inside load_instance_class, and
>>>> // the shared klass may not have been returned to us. Find it and make
>>>> // sure it's cleaned up.
>>>> if (class_loader.is_null()) {
>>>> instanceKlassHandle ik(THREAD, find_shared_class(name));
>>>> if (ik.not_null()) {
>>>> MutexLocker mu(SystemDictionary_lock, THREAD);
>>>> Klass* kk = find_class(name, loader_data);
>>>> if (kk == NULL) {
>>>> clean_up_shared_class(ik, class_loader, THREAD);
>>>> }
>>>> }
>>>> }
>>>> }
>>>>
>>>> if (load_instance_added == true) {
>>>> ....
>>> I like the approach of checking if this is in the system dictionary, that is a great idea.
>>> It needs this minor modification.
>>> The find_class that you are doing is the name/initiating loader pair.
>>> If the define_class worked for the name/defining loader (null for this case) that class
>>> may have been added to the systemdictionary for lookup, and in that case you do not
>>> want to do this cleanup.
>>> So the find class would be the defining loader - which would be the ik->class_loader_data()
>>> rather than the loader_data you were called with, which is the initiating loader.
>> I just want to understand a little more ...
>>
>> From the code, it seems the local variables "class_loader" and "loader_data" won't be changed after these lines inSystemDictionary::resolve_instance_class_or_null():
>>
>> class_loader = Handle(THREAD, java_lang_ClassLoader::non_reflection_class_loader(class_loader()));
>> ClassLoaderData *loader_data = register_loader(class_loader, CHECK_NULL);
>>
>> (or, is this true? Will the oop contained in the class_loader handle be modified subsequently??)
> I agree they will not be modified.
> You are absolutely accurate that this particular case will only happen if the initiating loader ==
> the defining loader == null class loader. Totally agree. So this code today will work as you propose it.
>>
>> Would this means, after the update_dictionary() call finishes, for a shared classes,
>>
>> ik->class_loader() == class_loader
>> == ClassLoaderData::the_null_class_loader_data()->class_loader()
>>
>> I can change the code to the way you like, but I need to add a check to make sureik->class_loader_data()has been initialized (just in case, in the future, we might get an exception before Klass::restore_unshareable_info() is even called):
>>
>> if (HAS_PENDING_EXCEPTION) {
>> // An OOM could have happened at various places inside load_instance_class, and
>> // the shared klass may not have been returned to us. Find it and make
>> // sure it's cleaned up.
>> if (class_loader.is_null()) {
>> instanceKlassHandle ik(THREAD, find_shared_class(name));
>> if (ik.not_null()) {
>> if (ik->class_loader_data() == NULL) {
>> // We didn't go as far asKlass::restore_unshareable_info(),
>> // so nothing to clean up
>> } else {
>> MutexLocker mu(SystemDictionary_lock, THREAD);
>> Klass* kk = find_class(name, ik->class_loader_data());
>> if (kk == NULL) {
>> clean_up_shared_class(ik, class_loader, THREAD);
>> }
>> }
>> }
>> }
>>
>> What do you think?
> I like this much better - thank you. I this will help prevent other problems in the future (if this were to be enhanced
> for other class data sharing, this would not trip up the person making those changes).
> (and the clean_up_shared_class would use the same ik->class_loader_data() argument as well).
>
> thanks,
> Karen
>> Thanks
>> - Ioi
>>
>>>>> So I'm assuming you are doing a cleanup of the unshareable info because:
>>>>> find_shared_class succeeded, resolve_super_or_fail on superclasses/interfaces succeeded and
>>>>> restore_unshareable_info succeeded.
>>>>> Can we assume load_shared_class succeeded or do we need a check there in case notify_class_loaded
>>>>> failed?
>>>>>
>>>>> For the bootloader today, LOAD_INSTANCE is not done in parallel, so we are not going to run
>>>>> into the situation in which another thread is loading the same class in the LOAD_INSTANCE sense.
>>>>> That said, once we add the instanceKlass to the systemdictionary - any other thread could
>>>>> find it and use it - so we don't want to clean it up - it may be in use.
>>>>>
>>>>> In load_instance_class if load_shared_class succeeds, you call find_or_define_instance_class, which
>>>>> calls define_class.
>>>>> Inside find_or_define_instance_class and define_instance_class you can have failures
>>>>> before you add to the systemdictionary - in which case you want to do this cleanup. If the failures are
>>>>> after that point - this thread may not get the information back due to a pending exception, but other
>>>>> threads may use the information - so you don't want to do the cleanup. So the key is - if update_dictionary
>>>>> succeeded - don't do the cleanup. If it did not, you want the cleanup.
>>>>>
>>>>> Coleen is planning to put in code to cleanup the instanceKlass - and this should go the same place I
>>>>> would assume, so you probably want to discuss this closely with her.
>>> Note that Coleen will also need cleanup for the resolve_from_stream path - which you don't need,
>>> around the find_or_define call.
>>>> OK, I will do that.
>>>>
>>>> Thanks for the detailed review! I really appreciate that.
>>>>
>>>> - Ioi
>>> thanks for jumping in on this complex code,
>>> Karen
>>>>> thanks,
>>>>> Karen
>>>>>
>>>>> On Mar 14, 2013, at 1:49 AM, Ioi Lam wrote:
>>>>>
>>>>>> Hi, could someone review this? This fixes a crash related to OOM handling.
>>>>>>
>>>>>> Thanks
>>>>>> - Ioi
>>>>>>
>>>>>> On 03/11/2013 12:47 PM, Ioi Lam wrote:
>>>>>>> Please review:
>>>>>>> http://cr.openjdk.java.net/~iklam/8007725/null_mirror_001/
>>>>>>>
>>>>>>> Bug: NPG: Klass::restore_unshareable_info() triggers
>>>>>>> assert(k->java_mirror() == NULL)
>>>>>>>
>>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8007725
>>>>>>> or https://jbs.oracle.com/bugs/browse/JDK-8007725
>>>>>>>
>>>>>>> (The bugs.sun.com page may be unavailable for a couple of days due
>>>>>>> to recent change of confidentiality status.)
>>>>>>>
>>>>>>> Summary of fix:
>>>>>>>
>>>>>>> The assert is at:
>>>>>>>
>>>>>>> --------
>>>>>>> # Internal Error (src/share/vm/classfile/javaClasses.cpp:527)
>>>>>>> # assert(k->java_mirror() == NULL) failed: should only assign mirror once
>>>>>>> java_lang_Class::create_mirror(KlassHandle)
>>>>>>> Klass::restore_unshareable_info()
>>>>>>> InstanceKlass::restore_unshareable_info()
>>>>>>> instanceKlassHandle SystemDictionary::load_shared_class(instanceKlassHandle,Handle)
>>>>>>> --------
>>>>>>>
>>>>>>> This happens if during class loading, an OOM happened after
>>>>>>> Klass::restore_unshareable_info() has called
>>>>>>> java_lang_Class::create_mirror(), but before the class loading is completed.
>>>>>>>
>>>>>>> At this point, the class is not in the system dictionary. However the
>>>>>>> Klass structure is partially initialized.
>>>>>>>
>>>>>>> If the app tries to load the same class again, the assert happens.
>>>>>>>
>>>>>>> The fix is to undo all work performed by
>>>>>>> InstanceKlass::restore_unshareable_info().
>>>>>>>
>>>>>>> Tests executed:
>>>>>>>
>>>>>>> * JPRT
>>>>>>> * JCK (vm, lang, api/signaturetest)
>>>>>>> * UTE (vm.quick.testlist, )
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Ioi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130314/8e9f7974/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list