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

[coleen.phillimore at oracle.com]

Dean, Thank you for the review and comments.
Coleen

On 10/26/18 3:06 PM, dean.long at oracle.com wrote:
> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>