RFR: 8210233: Prepare Klass::is_loader_alive() for concurrent class unloading
erik.osterlund at oracle.com
Mon Sep 10 10:28:37 UTC 2018
Thanks for the review.
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.
> Looks good to me.
>> 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.
>>>> 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
>>> 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.
More information about the hotspot-runtime-dev