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

[Erik Österlund]

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.

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