RFR: 8210233: Prepare Klass::is_loader_alive() for concurrent class unloading

[Per Liden]

Hi Erik,

On 09/04/2018 10:14 AM, Erik Österlund wrote:
> Hi Kim,
> 
> Thank you for the review.
> 
> Incremental:
> http://cr.openjdk.java.net/~eosterlund/8210233/webrev.01_02/
> 
> Full:
> http://cr.openjdk.java.net/~eosterlund/8210233/webrev.02/

Looks good to me.

/Per

> 
> On 2018-09-04 05:51, Kim Barrett wrote:
>>> On Sep 3, 2018, at 11:25 AM, Erik Österlund 
>>> <erik.osterlund at oracle.com> wrote:
>>>
>>> Hi Kim,
>>>
>>> You are right. Thank you for catching that.
>>>
>>> Incremental:
>>> http://cr.openjdk.java.net/~eosterlund/8210233/webrev.00_01/
>>>
>>> Full:
>>> http://cr.openjdk.java.net/~eosterlund/8210233/webrev.01/
>>>
>>> Thank you for the review.
>> The comments I was referring to were these:
>>
>> is_loader_alive in klass.hpp
>> 657   // Iff the class loader (or mirror for unsafe anonymous classes) 
>> is alive the
>> 658   // Klass is considered alive.  Has already been marked as 
>> unloading.
> 
> Fixed.
> 
>> classLoaderHierarchyDCmd.cpp
>> 471     // We do not display unloading loaders, for now.
>>
>> I'm once again finding myself confused about the relationship between
>> alive or not and unloading or not. Is the second sentence of the
>> comment for is_loader_alive even true now?
> 
> To me, the term "unloading" refers to a conceptual state the CLD is in 
> from when the marking terminates and the CLD holder is dead, to when it 
> is finally deleted (despite having to go through a number of operations 
> to get there, like (sometimes) reference processing, setting CLD state 
> to _unloading (which is merely a cache for this liveness information), 
> unlinking and deleting.
> 
> So I think the comment looks good (to me), but understand if there are 
> other interpretations by others reading it. Perhaps if we removed the 
> _unloading state altogether, things would be less confusing. It is only 
> a cache of !is_alive(), and its existence builds on the premise that 
> performing a phantom load on the CLD holder is expensive, and that it is 
> therefore worth caching if the result is NULL or not. But I do not think 
> that optimization gives us much. It does however constrain the order in 
> which you can do things, which can be problematic and confusing. But 
> that feels like a separate RFE.
> 
>> The new comment for is_alive is fine.
>>
>> In the new comment for is_unloading, the use of "unloaded" is
>> confusing me.  Is that a typo for "unloading", or am I confused?
> 
> That was a typo indeed. It is fixed now.
> 
>> I think I need to retract my review; I think I need to refresh and
>> de-confuse my understanding of this area.  Don't wait for me if you
>> get a second okay from someone else.
> 
> No worries Kim, there is no rush with this one.
> 
> Thanks,
> /Erik
>