RFR 8212958: Allow Klass::_subklass and _next_sibling to have unloaded classes

[dean.long at oracle.com]

On 10/26/18 8:04 AM, Erik Österlund wrote:
> Hi Coleen,
>
> On 2018-10-26 14:09, coleen.phillimore at oracle.com wrote:
>>
>> Hi Dean,  Thank you for reviewing this.
>>
>> On 10/26/18 2:17 AM, dean.long at oracle.com wrote:
>>> Hi Coleen.  Most of this looks good to me, but I have a few questions:
>>>
>>> Is the need for Compile_lock around accesses new in 12, because the 
>>> implementor and subclass links can now be cleaned without being at a 
>>> safepoint?
>>
>> Yes.  Erik's going to take Compile_lock to clean these during 
>> concurrent class unloading.  There were some unlocked accesses though 
>> that seemed suspicious before though.
>>>
>>> Style typo: In ciInstanceKlass::compute_has_subklass(), I think it 
>>> should be return "ik->subklass() != NULL".
>>
>> Fixed.
>>>
>>> I see that you removed the memoizing of _has_subklass but left 
>>> _implementor.  I'm concerned that this might have a performance 
>>> impact and change behavior.  It seems to allow has_subklass() to 
>>> change from true to false (and is_leaf_type() to change from false 
>>> to true) where previously that should have been impossible.  With 
>>> some fiddling with the locking, I was able to add back 
>>> _has_subklass.  Do you remember why you removed it?
>>
>> Yes, I removed the memoizing for _has_subklass because I couldn't 
>> take the Compile_lock in the ciInstanceKlass constructor, because 
>> some callers had it.   If you have a change that will make that work, 
>> I'd be happy to use yours.  I was trying to not change anything 
>> semantically, just grab fresh results from InstanceKlass which I 
>> didn't think would affect performance (although it grabs Compile_lock 
>> so maybe it could).   We can revert this part and add locking to 
>> ciInstanceKlass constructor.  I've been trying to avoid conditional 
>> locking.
>
> Can you really safely rely on a cached boolean w.r.t. unloading? The 
> instant we release the mark end safepoint, compiler threads will come 
> asking if there are subklasses, before we have started doing 
> unloading, and they have to get consistent answers that there are no 
> subklasses, if the one subklass had died. So it seems to me that this 
> value must be computed and not cached. You can however use the same 
> trick I used for CompiledMethod::is_unloading in 8212958, and make the 
> answer belong to an epoch. That way, we force computation for the 
> first time only that the question is asked, and use the cached result 
> that we know to be consistent w.r.t. unloading in subsequent calls. 
> But I wonder if we are micro optimizing by doing that here.
>

The compiler tasks protect against has_subklass changing from false to 
true by checking SystemDictionary::_number_of_modifications. 
Optimizations are done when has_subklass is false, so if it changes from 
true to false, then we may have missed some optimizations before the 
change, but we don't have to throw out the generated code.  Do we need 
to make changes here for concurrent unloading?

dl

> Thanks,
> /Erik
>
>> Thanks,
>> Coleen
>>>
>>> dl
>>>
>>> On 10/25/18 1:42 PM, coleen.phillimore at oracle.com wrote:
>>>> Summary: Don't return unloaded klasses. Make sure access is 
>>>> protected by Compile_lock.
>>>>
>>>> See bug for more description.  This test has compiler changes since 
>>>> the weak klass links in Klass (and implementor in InstanceKlass are 
>>>> used by the compiler).  All the compiler jtreg tests passed.
>>>>
>>>> Tested with mach5 tier1-7 and test added.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8212958.01/webrev
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8212958
>>>>
>>>> Thanks,
>>>> Coleen
>>>
>>
>