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

[Coleen Phillimore]

On 6/16/14, 7:57 PM, David Holmes wrote:
> On 17/06/2014 12:38 AM, 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.
>
> That sounds bad them. If Thread A holds the lock, Thread B will block 
> on it but not be safepoint-safe; if Thread A then initiates a 
> safepoint via GC then the safepoint won't be reachable.

Yes, this is bad and the deadlock I got by not making this lock have 
no_safepoint_check.
>
>> 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.
>
> Relatedly every block of code that takes a lock without a safepoint 
> check should have a NO_SAFEPOINT_VERIFIER. I wonder if that can be 
> built in to the lock_without_safepoint_check?

So I did a bit of research and we use no_safepoint_check because we call 
this during GC (rather than doing GC while holding this lock).

But that's a good idea about adding No_Safepoint_Verifier.  We could 
subclass MutexLockerEx with a NSV field.  I could file an RFE with this 
suggestion.  I don't think I want to add a lot of extra code to this change.

Did you review the code?

Coleen

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