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

[Ioi Lam]

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?

The old code in constantPool.cpp allocated a Monitor, not a Mutex:

   set_lock(new Monitor(Monitor::nonleaf + 2, "A constant pool lock"));

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