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

[coleen.phillimore at oracle.com]

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
>>>>
>>>
>>
>