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

[David Holmes]

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.

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

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