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

[Ioi Lam]

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/20130321/1a781676/attachment.html