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

[Erik Österlund]

Hi Dean,

Thank you for the review!

/Erik

On 2018-11-27 07:11, dean.long at oracle.com wrote:
> 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
>>>
>>
>