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

[Erik Österlund]

Hi Per,

Thanks for the review.

/Erik

On 2018-09-10 12:20, Per Liden wrote:
> 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
>>