RFR (XS) 8010723 - possible deadlock with SystemDictionary_lock

[David Holmes]

On 3/04/2013 8:44 AM, Ioi Lam wrote:
> Hi David,
>
> Sorry I didn't have time to incorporate your comment into the patch --
> since it was a P2, I was in a hurry and committed the patch before
> seeing your comments.

That's okay.

> It will probably be something like:
>
>     assert(class_loader.is_null() || !is_parallelCapable(class_loader),
> "sanity");
>     clean_up_shared_class(ik, class_loader, THREAD);
>
> But wait ... CDS would need a lot of changes if we were to allow
> non-boot ClassLoaders to load from the CDS image (I am working on such a
> prototype).
>
> Let's say C is a class in the CDS image. We have a parallel-capable
> ClassLoader instance Foo. Two threads, T1 and T2, are trying to load the
> same Klass C into Foo. We cannot allow both threads to work on the same
> copy of C, so we have two choices:
>
>      1. Make it single-threads (T2 wait for T1)
>
> or
>
>      2. T1 loads C from CDS, but T2 loads C' from JAR file;
>         the thread that finishes first wins. This is probably the
>         correct way to preserve the parallel-capable property
>         of the ClassLoader.
>
> In either case, if we need to clean up C due to OOM, we can guarantee
> that no other thread will be holding a copy of C.
>
> So, maybe we don't need such an assert after all. What do you think?

I'm not certain how loading from the shared archive is currently done, 
but I would expect the archive itself is read-only hence no concurrency 
issues. Using parallel-define-class two threads both loading the same 
class will eventually be serialized due to locking in the 
systemDictionary - at which point first to load wins and the second then 
takes the result produced by the first. I'm not sure that presence of 
CDS archive would/should change that story.

David
-----

> Thanks
> - Ioi
>
>
> On 04/01/2013 07:08 PM, David Holmes wrote:
>> Is there any way to express this:
>>
>> 839   // This need to be re-thought when parallel-capable non-boot
>> 840   // classloaders are supported by CDS (today they're not).
>>
>> as an assert?
>>
>> David
>>
>> On 30/03/2013 5:26 AM, Ioi Lam wrote:
>>> Coleen,
>>>
>>> I have updated the webrev to incorporate your comments:
>>>
>>> http://cr.openjdk.java.net/~iklam/8010723/sysdic_lock_002/
>>>
>>> Thanks
>>> - Ioi
>>>
>>> On 03/29/2013 10:38 AM, Coleen Phillimore wrote:
>>>>> // This must be done outside of the SystemDictionary_lock to
>>>>>     // avoid deadlock.
>>>>>     //
>>>>>     // Note that Klass::restore_unshareable_info (called via
>>>>>     // load_instance_class above) is also called outside
>>>>>     // of SystemDictionary_lock. All other threads that are
>>>>>     // trying to load this class in the boot classloader
>>>>>     // are already blocked above at
>>>>>     // if (class_loader.is_null()) {SystemDictionary_lock->wait();}
>>>> How about
>>>>
>>>>       // Other threads are blocked from loading this class because
>>>> they are
>>>>       // are waiting on the SystemDictionary_lock until this thread
>>>> removes
>>>>       // the placeholder below.
>>>>
>>>> thanks,
>>>> Coleen
>>>>>     //
>>>>>     // This need to be re-thought when parallel-capable non-boot
>>>>>     // classloaders are supported by CDS (today they're not).
>>>
>