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

[dean.long at oracle.com]

Looks good to me.

dl

On 10/26/18 11:20 AM, coleen.phillimore at oracle.com wrote:
>
> Hi, I have tested a version that memoizes _has_subklass in 
> ciInstanceKlass on first use, which doesn't require locking in the 
> ciInstanceKlass constructor.
>
> http://cr.openjdk.java.net/~coleenp/8212958.02/webrev/index.html
>
> Thanks,
> Coleen
>
>
> On 10/26/18 1:24 PM, dean.long at oracle.com wrote:
>> 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
>>>>>
>>>>
>>>
>>
>