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

[David Holmes]

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.

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

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