RFR: 8213209: [REDO] Allow Klass::_subklass and _next_sibling to have unloaded classes

[coleen.phillimore at oracle.com]

Erik, thank you for taking this change and making it lock-free.

On 11/16/18 1:12 PM, Erik Österlund wrote:
> Hi,
>
> This is a redo of Coleen's earlier patch to clean weak metadata links 
> using the Compile_lock.
>
> We noticed the proposed solution could cause deadlocks. At least one 
> of the reasons for that is that the Compile_lock was taken during 
> printing and verification in safepoints. And the Compile_lock is 
> sometimes held while issuing safepoints. The latter problem also 
> forces us to take the Compile_lock before joining the suspendible 
> thread set, to avoid another deadlock situation. But that is really 
> nasty for other reasons. And there have been a few more problems as well.
>
> So in this case, we think it is both safer, less error prone, and 
> better performing (although less important) to perform this cleaning 
> in a lock-free fashion rather than trying to dodge all the issues 
> related to the locking protocol.
>
> Inserts still require a lock. Therefore, we are dealing with lock-free 
> reads, and lock-free cleaning. The lock-free cleaning is interacting 
> with multiple lock-free readers and a single writer.
>
> The single inserter always helps with cleaning the subclass list head 
> before prepending new nodes to the chain. That allows an invariant 
> that the siblings link is never inserted pointing at a Klass that is 
> unloading, which simplifies things a lot. The head is inserted in a 
> classic load + CAS in a loop.
>
> Readers require load_acquire when reading the head, due to competing 
> inserts putting new Klass pointers there. The siblings, however, only 
> need relaxed consistency, because they are linked to data strictly 
> older than the head, which has already been acquired.
>
> Unlinked entries are all inserted into a purge list, because freeing 
> them immediately is not safe. A global handshake operation is 
> performed, and after that ClassLoaderDataGraph::purge() will go 
> through the list and delete the entries safely.

I don't see the purge list in your change, and it's not needed since 
this is unlinking in place.  I think this is your other change you're 
thinking of.

Should the Compile_lock be removed in ciInstanceKlass.cpp
ciInstanceKlass* ciInstanceKlass::implementor() {

and in jvmciCompilerToVM.cpp
C2V_VMENTRY(jobject, getImplementor, (JNIEnv *, jobject, jobject 
jvmci_type))

The former does a bit more than calling implementor().  Or do you want 
to leave these locks to be conservative?

   JVMCIKlassHandle handle(THREAD);
   {
     // Need Compile_lock around implementor()
     MutexLocker locker(Compile_lock);
     handle = iklass->implementor();
   }


http://cr.openjdk.java.net/~eosterlund/8213209/webrev.00/src/hotspot/share/oops/instanceKlass.cpp.udiff.html

nof_implementors() has an assert_locked_or_safepoint(Compile_lock); 
which it no longer needs.  You'll hit this from printing because you've 
removed the locking here:

3093   if (is_interface()) {
3094 MutexLocker ml(Compile_lock);
3095     st->print_cr(BULLET"nof implementors:  %d", nof_implementors());
3096     if (nof_implementors() == 1) {
3097       st->print_cr(BULLET"implementor:    ");
3098       st->print("   ");
3099       implementor()->print_value_on(st);
3100       st->cr();
3101     }


We don't test printing very well.

http://cr.openjdk.java.net/~eosterlund/8213209/webrev.00/src/hotspot/share/utilities/vmError.cpp.udiff.html

This file is unchanged except a line removed.

This change looks good.  I've run through the tests with it and didn't 
find any problems that doomed my earlier version.

Thanks!!
Coleen
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8213209/webrev.00/
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8213209
>
> Thanks,
> /Erik