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

[Coleen Phillimore]

On 3/17/2013 1:43 PM, Ioi Lam wrote:
> I have a kind of related questions: if locking of an oop is contended, 
> do we allocate a Monitor object for this oop? Will the Monitor be 
> eventually freed even if the oop is sill alive?

I don't know if the object monitor is deflated, or when.

>
> The old code in constantPool.cpp allocated a Monitor, not a Mutex:
>
>   set_lock(new Monitor(Monitor::nonleaf + 2, "A constant pool lock"));

Yes, I think it should have been a Mutex* because there's no 
wait/notify.  They're both the the same size.
Coleen

>
> Thanks
> - Ioi
>
> On 03/17/2013 10:33 AM, Coleen Phillimore wrote:
>>
>> Hi David,
>>
>> You reviewed the init_lock change.  I'm not sure if you reviewed the 
>> Mutex* that we added for the constant pool change as well. Both added 
>> footprint relative to permgen.   You were pretty dubious about 
>> zeroing out the init_lock too iirc.  More below:
>>
>> On 3/17/2013 3:43 AM, David Holmes wrote:
>>> On 17/03/2013 2:04 PM, Ioi Lam wrote:
>>>> On 03/16/2013 04:37 PM, David Holmes wrote:
>>>>> Hi Ioi,
>>>>>
>>>>> On 16/03/2013 5:07 PM, Ioi Lam wrote:
>>>>>> Please review:
>>>>>> http://cr.openjdk.java.net/~iklam/8008962/constpool_lock_001/
>>>>>> <http://cr.openjdk.java.net/%7Eiklam/8008962/constpool_lock_001/>
>>>>>>
>>>>>> Bug: 8008962: NPG: Memory regression: One extra Monitor per 
>>>>>> ConstantPool
>>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8008962
>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8008962 (Oracle Internal)
>>>>>>
>>>>>> Summary of fix:
>>>>>>
>>>>>> The fix is implemented according to Coleen's suggestion in the bug's
>>>>>> comments.
>>>>>>
>>>>>> Instead of using a Monitor for each ConstantPool (~150 bytes per
>>>>>> class), I re-use InstanceKlass::init_lock() for locking the
>>>>>> ConstantPool.
>>>>>>
>>>>>> init_lock used to be cleared after the class had been initialized.
>>>>>> There was complicated code that needed to deal with the clearing
>>>>>> (using volatile, OrderAccess::storestore(), etc). This code is no
>>>>>> longer necessary and has been removed.
>>>>>
>>>>> But the whole point of removing it was that we did not want the extra
>>>>> memory usage to exist for the lifetime of the class! This will
>>>>> increase dynamic footprint in a potentially unacceptable way.
>>>>>
>>>> We need a lock for the lifetime of the ConstantPool (see below), so 
>>>> I am
>>>> just re-using the init_lock and make it live forever. If I remove the
>>>> init_lock then I have to create another lock object.
>>>
>>> I understand, but the need for a new lock object is problematic. 
>>> With the permgen removal the klass stopped being an oop and a new 
>>> object had to be introduced just for class initialization locking.  
>>> That was bad for dynamic footprint but we compromised by making the 
>>> lock object short-lived. Then we needed a lock-object for the CP. 
>>> This originally introduced a new lock object and that caused this 
>>> footprint issue. So you are proposing to re-use the init-lock, but 
>>> given that was short-lived and will now be long lived, you have not 
>>> made any net gain in footprint in the long term.
>>
>> Relative to when we had permgen, this isn't a net gain, but the 
>> init_lock is much smaller than the Mutex* that we have in the 
>> Constant Pool.   If uncontended the init_lock is only 3 words, the 
>> mutex is something like 156 bytes.  So we have made net gain relative 
>> to the current sources.
>>
>>>
>>>>>>      The overhead is reduced to one int[0] per class.
>>>>>>
>>>>>> Because the lock is used at two different levels (class
>>>>>> initialization, and ConstantPool access), I thought about the
>>>>>> possibility of deadlocks.
>>>>>
>>>>> Top understand the deadlock potential I need to understand what locks
>>>>> the constant pool and why/when.
>>>>
>>>> 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?
>>>
>>
>> A good test would be to call class.forName in the <clinit> 
>> function.   This lock can be taken during initialization but if it's 
>> a reentrant lock, it should be okay.   I'm sure at least hundreds of 
>> tests already do this, but verifying this with a specific test and 
>> making sure would be a good thing to do.
>>
>> Coleen
>>>>> 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.
>>>
>>> David
>>> -----
>>>
>>>> Thanks
>>>> - Ioi
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> ------
>>>>>
>>>>>
>>>>>> However,
>>>>>>      it seems the only case of a deadlock would be some sort of 
>>>>>> circular
>>>>>> dependency
>>>>>>      in running <clinit> (as in the following example). However, 
>>>>>> in this
>>>>>> case, a
>>>>>>      deadlock would have already happened, just from grabbing the
>>>>>> initialization
>>>>>>      locks, before we would even have a chance to access the
>>>>>> ConstantPools. Hence,
>>>>>>      reusing the lock for ConstantPool access doesn't add any new 
>>>>>> paths
>>>>>> for deadlocks.
>>>>>>
>>>>>>      -------------------------------
>>>>>>      public class ConcurClinit {
>>>>>>          static void sleep(int ms) {
>>>>>>              try {Thread.sleep(ms);} catch (Throwable t) {;}
>>>>>>          }
>>>>>>          static class A {
>>>>>>              static {
>>>>>>                  sleep(1000);
>>>>>>                  B.xxx();
>>>>>>                  sleep(1000);
>>>>>>              }
>>>>>>              static void xxx() {}
>>>>>>          }
>>>>>>          static class B {
>>>>>>              static {
>>>>>>                  sleep(1000);
>>>>>>                  A.xxx();
>>>>>>                  sleep(1000);
>>>>>>              }
>>>>>>              static void xxx() {}
>>>>>>          }
>>>>>>          public static void main(String args[]) {
>>>>>>              (new Thread() {
>>>>>>                      public void run() {
>>>>>>                          A a = new A();
>>>>>>                      }
>>>>>>                  }).start();
>>>>>>              (new Thread() {
>>>>>>                      public void run() {
>>>>>>                          B b = new B();
>>>>>>                      }
>>>>>>                  }).start();
>>>>>>          }
>>>>>>      }
>>>>>>      -------------------------------
>>>>>>
>>>>>> Tests executed:
>>>>>>
>>>>>>      * JPRT
>>>>>>      * JCK (vm, lang, api/signaturetest)
>>>>>>      * UTE (vm.runtime.testlist, vm.quick.testlist,
>>>>>> vm.parallel_class_loading.testlist
>>>>>> )
>>>>>>
>>>>>> Thanks,
>>>>>> Ioi
>>>>
>>
>