RFR (M) 8026977: NPG: Remove ConstantPool::lock

[Coleen Phillimore]

On 6/16/14, 2:45 AM, David Holmes wrote:
> On 13/06/2014 6:58 AM, Coleen Phillimore wrote:
>>
>> Thank you, John!  Christian suggested I take out ConstantPool::lock()
>> and while doing so I discovered I was still using the unsafe
>> ConstantPool::unresolved_klass_at() call.  So I replaced these with
>> ConstantPool::klass_name_at() which is safe.
>>
>> Also, Sergey Kuksenko's tests exposed a deadlock with this code. Locking
>> the metaspace_lock() must not check for safepoints.
>
> Just to be paranoid ... so all user's of this lock elide the safepoint 
> check, and no use of this lock can encounter a safepoint within the 
> locked region?

Yes, to first question and I'm not sure to second.  I believe we can 
have a metaspace lock and call GC.  The reason it's a no-safepoint-check 
lock is that it can be taken during GC.  Which sounds unnervingly 
circular.  I'll have to reconstruct why this lock was done this way.

I wish there was a consistency check though.  If you take a lock without 
checking for safepoint, we should give an assertion if you take the same 
lock and check for safepoint.  Not sure if there's an easy way to code 
it, but I think it would be worth having.

Coleen

>
> David
> -----
>
>> I make Chris's changes and fixed these problems and retested.  Most
>> files are unchanged, but here is the updated webrev.
>>
>> I didn't mention before that this change saves 152 bytes per class.
>>
>> http://cr.openjdk.java.net/~coleenp/8026977_2/
>>
>> Thanks!
>> Coleen
>>
>> On 6/12/14, 12:57 PM, John Rose wrote:
>>> Reviewed.  This is a good change.  The code is much cleaner.  Thanks.
>>> — John
>>>
>>> On Jun 12, 2014, at 8:57 AM, Coleen Phillimore
>>> <coleen.phillimore at oracle.com> wrote:
>>>
>>>> On 6/12/14, 11:43 AM, Christian Thalinger wrote:
>>>>> +Mutex* ConstantPool::lock() {
>>>>> +  // Use the lock from the metaspace in the rare instance we need
>>>>> to lock the entries
>>>>> +  // in this cpool or its associated cache.  Only used for setting
>>>>> invokedynamic cpCache
>>>>> +  // entry.
>>>>> +  return pool_holder()->class_loader_data()->metaspace_lock();
>>>>> +}
>>>>>
>>>>> I’d rather see this method removed completely and use the metaspace
>>>>> lock explicitly in ConstantPoolCacheEntry::set_method_handle_common.
>>>>> Otherwise it's confusing.
>>>> Ok, I can change that.
>>>>
>>>> Coleen
>>>>
>>>>> On Jun 11, 2014, at 2:13 PM, Coleen Phillimore
>>>>> <coleen.phillimore at oracle.com <mailto:coleen.phillimore at oracle.com>>
>>>>> wrote:
>>>>>
>>>>>> Summary: Write klass and resolved_references constant pool fields
>>>>>> lock free.
>>>>>>
>>>>>> The constant pool values that can change were already read lock
>>>>>> free, through the cpSlot type (lsb set for Symbol vs. Klass) for
>>>>>> JVM_CONSTANT_{Unresolved}Class.   With Permgen elimination, the
>>>>>> other values that can change were moved to the resolved_references
>>>>>> array, which is initialized to null.  Non-null is the resolved
>>>>>> value.  With a couple uses of CAS, we can eliminate the need for
>>>>>> the constant pool lock for the constant pool changes. Error
>>>>>> handling also changes the tag but saving the resolution exception
>>>>>> was done under the SystemDictionary_lock, so only the tag change
>>>>>> needs a CAS.
>>>>>>
>>>>>> The only remaining use for the constant pool lock is updating the
>>>>>> cpCache for invokedynamic.  There are 4 fields that need to be
>>>>>> consistent.   These now use the metaspace lock associated with the
>>>>>> class loader that owns the constant pool, which is only held
>>>>>> briefly.   I ran some performance tests written by Sergey Kuksenko
>>>>>> which show no regression.
>>>>>>
>>>>>> Other testing - ran refworkload on linux x64 with no significant
>>>>>> results.  Passed JPRT (runThese), vm.quick.testlist, jck8 tests,
>>>>>> hotspot jtreg tests and jdk java/lang/invoke jtreg tests.
>>>>>>
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8026977/
>>>>>> <http://cr.openjdk.java.net/%7Ecoleenp/8026977/>
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8026977
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>>
>>>>>>
>>