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

[Coleen Phillimore]

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