RFR (S) JDK-8008962: NPG: Memory regression: One extra Monitor per ConstantPool
Ioi Lam
ioi.lam at oracle.com
Sun Apr 14 21:02:55 PDT 2013
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.
- 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