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

[David Holmes]

On 17/06/2014 11:45 AM, Coleen Phillimore wrote:
>
> 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).

OK.

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

Yes RFE would be good.

> Did you review the code?

No sorry, don't know the area enough.

David

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