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

[Coleen Phillimore]

RFE filed https://bugs.openjdk.java.net/browse/JDK-8047290

thanks,
Coleen

On 6/17/14, 6:11 AM, Mikael Gerdin wrote:
> On Monday 16 June 2014 10.38.08 Coleen Phillimore wrote:
>> 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.
> Can't we use the type system for this?
>
> Have a
> class NoSafepointCheckMutex which always elides safepoint checks
> and a
> class Mutex which does not allow elision of safepoint checks.
>
> /Mikael
>
>> 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