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

[David Holmes]

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?

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