RFR (S) JDK-8008962: NPG: Memory regression: One extra Monitor per ConstantPool
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Apr 16 12:30:37 PDT 2013
On 04/15/2013 12:02 AM, Ioi Lam wrote:
> On 04/14/2013 08:13 PM, David Holmes wrote:
>> 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
>>
> Oops, thanks for noticing. The comment is wrong. Per Karen's request,
> I have changed the name back to the original name (_init_lock). I will
> fix the comment when committing it.
This looks good to me. Can't find this comment about per_class_lock,
you must have fixed it...
Coleen
>
> - Ioi
>
>> ??
>>
>> 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