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

[Christian Thalinger]

Looks good.

On Jun 12, 2014, at 1:58 PM, Coleen Phillimore <coleen.phillimore at oracle.com> 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.
> 
> 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
>>>>> 
>>>>> 
>