RFR (XS) 8010723 - possible deadlock with SystemDictionary_lock

[David Holmes]

On 4/04/2013 4:26 PM, Ioi Lam wrote:
> On 04/03/2013 10:07 PM, David Holmes wrote:
>> 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.
>
> Part of the CDS archive is mapped RW -- specifically, all the
> InstanceKlasses are RW. The contents of a InstanceKlass is updated when
> it's being loaded (see InstanceKlass::restore_unshareable_info). So we
> cannot allow two concurrent threads to modify it at the same time.

Okay so that just requires a lock on updating the archive. No different 
to the fact the SD needs locking for concurrent loads.

David
-----

> - Ioi