RFR (S) JDK-8008962: NPG: Memory regression: One extra Monitor per ConstantPool

[Coleen Phillimore]

Ioi,

I think this is a good change.  On inspection it saves space and we have 
to have a lock, so a measurement is good if you can get it but 
otherwise, it's still a nice cleanup.

Thanks!
Coleen

On 3/22/2013 1:12 AM, Ioi Lam wrote:
> Hi folks,
>
> I have updated the patch. Please review
>
> http://cr.openjdk.java.net/~iklam/8008962/constpool_lock_002/ 
> <http://cr.openjdk.java.net/%7Eiklam/8008962/constpool_lock_002/>
>
> The only change is to check if the lock is not yet initialized. This 
> happens only during class file parsing, so locking is not necessary.
>
>       oop cplock = this_oop->lock();
>       ObjectLocker ol(cplock , THREAD, cplock != NULL);
>
> Thanks
> - Ioi
>
>
> On 03/18/2013 09:32 AM, Ioi Lam wrote:
>> On 03/17/2013 12:43 AM, David Holmes wrote:
>>>
>>>> There are various places such as ConstantPool::klass_at_impl that need
>>>> to make atomic modifications of an CP entry and its corresponding tag.
>>>> These can be called well after the class has finished initialization.
>>>
>>> The question is more, can they be called before or during class 
>>> initialization?
>>>
>>
>> Klass::init_lock is initialized in ClassFileParser::parseClassFile(). 
>> However, the CP is created before this. So there's a chance that the 
>> CP may try to lock on ConstantPool::lock() before Klass::init_lock() 
>> is initialized (or even before ConstantPool::_pool_holder is 
>> initialized).
>>
>> Nevertheless, I have not (yet) seen this happening with a fair amount 
>> of stress tests.
>>
>> Also, up to the initialization of Klass::init_lock(), only the 
>> ClassFileParser has a reference to the InstanceKlass and the 
>> ConstantPool, so everything is single threaded. I will change the 
>> code to be something like this (similar to what was done in 
>> InstanceKlass with the init_lock):
>>
>>      oop cplock = lock();
>>      ObjectLocker ol(cplock, THREAD, cplock != NULL);
>>
>>> - if we don't need to inflate (do we have any stats on this?) then 
>>> we don't get any overhead beyond the int[0] 
>>
>> I don't have any stats. How would one go about collecting the locking 
>> stats on specific objects?
>>
>> Looking at the code, most use of the lock would be in 
>> ConstantPool::klass_at_impl(), and only if the slot is still an 
>> unresolved class. Also, the lock is usually held for a very short 
>> period of time, unless you hit an exception, or hit a GC at this block
>>
>>       MonitorLockerEx ml(this_oop->lock());
>>       // Only updated constant pool - if it is resolved.
>>       do_resolve = this_oop->tag_at(which).is_unresolved_klass();
>>       if (do_resolve) {
>>         ClassLoaderData* this_key = 
>> this_oop->pool_holder()->class_loader_data();
>>         this_key->record_dependency(k(), CHECK_NULL); // Can throw 
>> OOM  <<<<<< GC may happen here
>>         this_oop->klass_at_put(which, k());
>>       }
>>
>> So my wild guess is you rarely would get a contention on the lock.
>>
>>>>> Is there a possibility of a self-deadlock if during class
>>>>> initialization we have to lock the constant-pool ourselves?
>>>> The locking is done using ObjectLocker with an oop, so it is self
>>>> reentrant, just like a regular Java monitor entry. Unlike mutexes, 
>>>> there
>>>> won't be self deadlocks.
>>>
>>> Okay. But recursive locking can also be problematic if you don't 
>>> fully understand the circumstances under which it can occur - 
>>> because you effectively lose atomicity relative to actions in the 
>>> current thread.
>>
>> Sorry I don't quote understand this. Could you explain more?
>>
>> Thanks a lot!
>>
>> - Ioi
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130322/5230ed77/attachment-0001.html