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

[dean.long at oracle.com]

Looks OK to me too.  The lazy initialization of _has_subklass probably 
isn't needed anymore, but it does seem to be an improvement.

dl


On 11/19/18 4:52 AM, Erik Österlund wrote:
> 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
>>
>