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

[Mikael Gerdin]

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