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

[Erik Österlund]

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/

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