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

[Erik Österlund]

Hi Coleen,

Thanks for the review.

Full webrev:
http://cr.openjdk.java.net/~eosterlund/8213209/webrev.01/

Incremental webrev:
http://cr.openjdk.java.net/~eosterlund/8213209/webrev.00_01/

On 2018-11-16 22:08, coleen.phillimore at oracle.com wrote:
>
> 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.

Yes you are right, of course. It's the unloading list and not a separate 
purge list...

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

While I think your analysis is correct, I would indeed like to leave 
them in this RFE 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.
>

Ahh yes - good catch. I removed the assert as it is no longer required.

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

I put it back the way it was before.

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

Thank you for running additional tests on this patch!

Thanks,
/Erik

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