RFR 8028497: SIGSEGV at ClassLoaderData::oops_do(OopClosure*, KlassClosure*, bool)
Ioi Lam
ioi.lam at oracle.com
Thu Mar 27 22:30:13 UTC 2014
Got it. The rest of the changes look good to me.
Thanks!
- Ioi
On 3/27/14, 2:33 PM, Coleen Phillimore wrote:
>
> On 3/27/14 3:01 PM, Ioi Lam wrote:
>> Coleen,
>>
>> Why is this change necessary for this bug anyway?
>>
>> http://cr.openjdk.java.net/~coleenp/8028497_2/src/share/vm/classfile/classFileParser.cpp.sdiff.html
>>
>>
>> ClassFileParser is not used when loading a class from CDS archive.
>
> It is to delay actually deallocating the InstanceKlass until a
> safepoint, so that the mirror can point to it until then. But that
> might not be long enough. We might need to scribble over the mirror
> too (see other emails). It's also the only other case that unlinks
> the Klass from the CLD::_klasses list outside a safepoint. That's why
> it was included in this fix, because it also helps CMS code.
>
> Coleen
>
>>
>> - Ioi
>>
>> On 3/27/14, 10:11 AM, Erik Helin wrote:
>>> Hi Coleen,
>>>
>>> thanks for taking on this tricky bug! However, I think there is an
>>> issue with your fix :(
>>>
>>> This case is for "normal" (non-CDS) class loading:
>>> 1. We create the mirror
>>> 2. We set up the link from the mirror to the Klass
>>> 3. We fail somewhere before setting the link from the Klass to the
>>> mirror, for example when allocating the Java object for the init
>>> lock.
>>> 4. The destructor for ClassFileParser is run and we add the Klass to
>>> the
>>> deallocate list.
>>> 5. A CMS concurrent cycle is started, it will treat all objects in eden
>>> + the survivor area as roots (including the mirror).
>>> 6. The above concurrent cycle enters the "final remark" phase and does
>>> class unloading which will call free_deallocate_list() which will
>>> return the memory for the Klass added to the deallocate list in step
>>> 4 to Metaspace.
>>> 7. The concurrent CMS cycle ends (the mirror is still in eden or young)
>>> 8. A new concurrent CMS cycle is started, the mirror will once again be
>>> treated as a root since it is still in eden or young (no YC has been
>>> done).
>>> 9. During the initial marking, CMS will now follow the pointer to the
>>> Klass that the mirror is mirroring, but we returned this memory to
>>> Metaspace in step 6.
>>> 10. We now crash :(
>>>
>>> Unfortunately, I don't have any suggestion on how to solve the above
>>> scenario at the moment.
>>>
>>> Thanks,
>>> Erik
>>>
>>> On 2014-03-27 16:04, Coleen Phillimore wrote:
>>>>
>>>> On 3/27/14 8:39 AM, Stefan Karlsson wrote:
>>>>>
>>>>> On 2014-03-27 03:00, Coleen Phillimore wrote:
>>>>>>
>>>>>> I have a new version of this change based on code that Ioi noticed
>>>>>> that I was missing for making restoring the method's adapters
>>>>>> restartable if something fails during restore_unshareable_info, and
>>>>>> the constant pool resolved references array.
>>>>>> Please review this new code (only method.* and instanceKlass.cpp and
>>>>>> constantPool.cpp have differences). Reran tests with CDS and CMS.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~coleenp/8028497_2/
>>>>>
>>>>> This seems good. I'd prefer if someone with more experience with CDS
>>>>> also Reviews the change.
>>>>
>>>> Thanks, Ioi said he'd look at it.
>>>>
>>>>>
>>>>> Small comment:
>>>>>
>>>>> http://cr.openjdk.java.net/~coleenp/8028497_2/src/share/vm/oops/instanceKlass.cpp.udiff.html
>>>>>
>>>>>
>>>>>
>>>>> - methodHandle m(THREAD, methods->at(index2));
>>>>> - m()->link_method(m, CHECK);
>>>>> - // restore method's vtable by calling a virtual function
>>>>> - m->restore_vtable();
>>>>> + Method *m = methods->at(index2);
>>>>> + m->restore_unshareable_info(CHECK);
>>>>>
>>>>>
>>>>> Do we really want to use a raw Method * here. I know that we wrap the
>>>>> method in a handle inside m->restore_unshareable_info, but it's not
>>>>> clear from the this context. I'm thinking about this issue:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8004815
>>>>>
>>>>> On the other hand, I guess we can't redefine a class before it has
>>>>> been defined. But maybe it would be good to not promote the usage of
>>>>> Method* over calls that might hit a Safepoint.
>>>>>
>>>>
>>>> I think you're right, we don't use 'm' after the potential
>>>> safepoint and
>>>> we can't really redefine this class yet anyway but I'll restore the
>>>> code
>>>> because we don't want to encourage uses of unhandled Methods unless
>>>> proven to be harmless.
>>>>
>>>> Thanks!
>>>> Coleen
>>>>> thanks,
>>>>> StefanK
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>>
>>>>>> On 3/26/14 5:15 PM, Coleen Phillimore wrote:
>>>>>>>
>>>>>>> Hi Jon,
>>>>>>> Thank you for looking at the code changes.
>>>>>>>
>>>>>>> On 3/26/14 12:24 PM, Jon Masamitsu wrote:
>>>>>>>> Coleen,
>>>>>>>>
>>>>>>>> Did you remove Class_obj_allocate() because it was only
>>>>>>>> called in that one spot and use obj_allocate() did most
>>>>>>>> of the work?
>>>>>>>
>>>>>>> I deleted it because GC doesn't need to know how these javaClasses
>>>>>>> are set up and I spent way to long looking for this code and didn't
>>>>>>> find it where it belongs, so I removed this. This
>>>>>>> Class_obj_allocate
>>>>>>> is an artifact of the permgen code.
>>>>>>>
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~coleenp/8028497/src/share/vm/oops/instanceMirrorKlass.cpp.frames.html
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 373 // the size in the mirror size itself.
>>>>>>>>
>>>>>>>> Drop the second "size". You know how we GC guys are
>>>>>>>> getting with our comments :-).
>>>>>>>
>>>>>>> Yes, I noticed that. Thanks - fixed.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~coleenp/8028497/src/share/vm/oops/klass.cpp.frames.html
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 498 void Klass::restore_unshareable_info(TRAPS) {
>>>>>>>> 499 // If an exception happened during CDS restore, some of
>>>>>>>> these fields may already be
>>>>>>>> 500 // set. We leave the class on the CLD list, even if
>>>>>>>> incomplete so that we don't
>>>>>>>> 501 // modify the CLD list outside a safepoint.
>>>>>>>> 502 if (class_loader_data() == NULL) {
>>>>>>>> 503 ClassLoaderData* loader_data =
>>>>>>>> ClassLoaderData::the_null_class_loader_data();
>>>>>>>>
>>>>>>>> I can see that this works with the current CDS (sharing only
>>>>>>>> classes loader
>>>>>>>> with the NULL class loader). Will it work if the shared classes
>>>>>>>> are loaded
>>>>>>>> with an application class loader?
>>>>>>>
>>>>>>> I hope Ioi can answer that. I assume so because the classes don't
>>>>>>> change class loaders if restoring shareable info fails.
>>>>>>>
>>>>>>> Ioi pointed out to me (not on open list) that I missed making
>>>>>>> link_method() and creating the constant pool resolved references
>>>>>>> array restartable, so I'm testing a fix for that also and I'll post
>>>>>>> an update later. But you've seen most of it.
>>>>>>>
>>>>>>> Thanks!!
>>>>>>> Coleen
>>>>>>>>
>>>>>>>> Rest looks good.
>>>>>>>>
>>>>>>>> Jon
>>>>>>>>
>>>>>>>> On 3/25/2014 1:42 PM, Coleen Phillimore wrote:
>>>>>>>>> Summary: Keep class in CLD::_klasses list and mirror created for
>>>>>>>>> CDS classes if OOM during restore_shareable_info(). This keeps
>>>>>>>>> pointers consistent for CMS.
>>>>>>>>>
>>>>>>>>> This change makes restoring the mirror for shared classes
>>>>>>>>> restartable if there is an OOM in the middle of
>>>>>>>>> restore_unshareable_info. The mirror field in the classes and
>>>>>>>>> CLD link is not cleaned up anymore. The class isn't marked
>>>>>>>>> loaded
>>>>>>>>> though so must be loaded again.
>>>>>>>>>
>>>>>>>>> Tested with Stefan's test case with hard coded OOM in jvm, so
>>>>>>>>> can't add the test. Also ran all the tests against -client and
>>>>>>>>> server with -Xshare:auto and with -XX:+UseConcMarkSweepGC.
>>>>>>>>>
>>>>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8028497/
>>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8028497
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Coleen
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>
More information about the hotspot-dev
mailing list