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

[Ioi Lam]

On 03/21/2013 10:46 PM, David Holmes wrote:
> This looks okay to me. Do we have updated Jetty figures to show the 
> memory regression has gone/reduced?
Where can I find instructions for running Jetty as a footprint 
benchmark? I tried searching on http://wiki.se.oracle.com but can't find 
anything definitive.

>
> Aside: Your webrev frames view is broken - the navigation frame gives 
> a 404 error.
>
Hmm, I will try to fix that.

Thanks
- Ioi

> 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
>>