RFR (S) JDK-8008962: NPG: Memory regression: One extra Monitor per ConstantPool
David Holmes
david.holmes at oracle.com
Sun Apr 14 20:13:38 PDT 2013
On 13/04/2013 4:52 AM, Ioi Lam wrote:
> Hi,
>
> I have updated the patch to reflect comments I received from Karen offline:
>
> http://cr.openjdk.java.net/~iklam/8008962/constpool_lock_003/
> <http://cr.openjdk.java.net/%7Eiklam/8008962/constpool_lock_003/>
>
> The changes from the last version are only comments and naming of
> fileds/methods. No programmatic changes.
I don't see the naming change referred to in the comment:
InstanceKlass::_init_lock (renamed to _per_class_lock) for locking
??
David
> Thanks
> - Ioi
>
> On 03/25/2013 02:08 PM, Ioi Lam wrote:
>> Jetty numbers before/after my fix (JDK is 8/b81)
>>
>> BEFORE
>> ============================================================================
>>
>> /scratch/iklam/jdk/tools/refworkload/130322/results.cplock_base
>> Benchmark Samples Mean Stdev Geomean Weight
>> footprint3_real 1 117032.00
>> jetty 1 117032.00
>> ============================================================================
>>
>>
>> AFTER
>> ============================================================================
>>
>> /scratch/iklam/jdk/tools/refworkload/130322/results.cplock
>> Benchmark Samples Mean Stdev Geomean Weight
>> footprint3_real 1 116728.00
>> jetty 1 116728.00
>> ============================================================================
>>
>>
>> So saving of about 300K.
>>
>> Also, for curiosity, I have tested jetty for the promoted JDK8 builds
>> from the past 6 months -- from /java/re, linux_amd64:
>>
>> b57, according to the bug report 8001590, is the last version that did
>> not have NPG
>> b78 has a big regression. It's fixed somewhat in b82 but still we are
>> much worse than b57.
>>
>> ==========================================================================
>>
>> Samples size stddev date
>> b55 6 96644.67 27.18 09/06/2012
>> b56 6 97334.00 26.50 09/13/2012
>> b57 6 97326.67 73.13 09/20/2012 << Last w/o NPG
>> b58 6 103212.00 70.70 09/27/2012
>> b59 6 103220.67 46.61 10/03/2012
>> b60 6 103187.33 102.68 10/11/2012
>> b61 6 100036.00 79.44 10/18/2012
>> b62 6 100020.67 78.77 10/25/2012
>> b63 6 100323.33 135.85 11/01/2012
>> b64 6 100297.33 76.72 11/08/2012
>> b65 6 101616.67 70.63 11/15/2012
>> b66 6 101445.33 71.63 11/29/2012
>> b67 6 101135.33 80.26 12/06/2012
>> b68 6 101596.00 89.66 12/13/2012
>> b69 6 101644.00 72.62 12/20/2012
>> b70 6 101716.00 106.19 12/27/2012
>> b71 6 101852.00 119.71 01/03/2013
>> b72 6 101844.67 144.12 01/10/2013
>> b73 6 102121.33 83.20 01/16/2013
>> b74 6 102473.33 61.43 01/24/2013
>> b75 6 101633.33 61.54 01/31/2013
>> b76 6 101760.00 65.73 02/07/2013
>> b77 6 101188.00 45.75 02/14/2013
>> b78 6 117358.67 154.79 02/21/2013 << big regression
>> b79 6 117236.67 58.97 02/28/2013
>> b80 6 117454.00 198.63 03/07/2013
>> b81 6 117308.67 30.61 03/14/2013
>> b82 6 106822.00 280.24 03/21/2013
>> ==========================================================================
>>
>>
>> - Ioi
>>
>> 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?
>>>
>>> Aside: Your webrev frames view is broken - the navigation frame gives
>>> a 404 error.
>>>
>>> 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
>>>>
>>
>
More information about the hotspot-runtime-dev
mailing list