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

David Holmes david.holmes at oracle.com
Thu Mar 21 22:46:19 PDT 2013


This looks okay to me. Do we have updated Jetty figures to show the 
memory regression has gone/reduced?

Aside: Your webrev frames view is broken - the navigation frame gives a 
404 error.

Thanks,
David

On 22/03/2013 3:12 PM, 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
>


More information about the hotspot-runtime-dev mailing list