RFR: 8218969: Free JVMCI native structures in nmethod::flush

[Erik Österlund]

Hi Tom and Doug,

Thank you for the reviews.

I inlined the deletion straight into nmethod::flush as Tom suggested:
http://cr.openjdk.java.net/~eosterlund/8218969/webrev.01/

Regarding the less aggressive jweak reclamation, I am convinced that 
will not be an observable problem. To me, the real problem is code 
complexity, corner cases and exceptions to our model. To me, it is more 
important to harmonize the nmethod life cycle than to save memory.

For some context, here is a list of properties that makes compiled 
methods have completely different lifecycles:

AOT vs nmethod, JVMCI nmethod vs normal nmethod, unloaded by GC vs 
deopt, OSR non-JVMCI nmethods vs normal nmethods (unloaded OSR nmethods 
never become zombie, but non-OSR nmethods do... except if they are JVMCI 
nmethods, then they don't become unloaded, they become not entrant), 
VM-locked unloaded vs zombie nmethods, etc. And then on top of this, the 
crazy safepointophobic GC folks made nmethods unload concurrently, and 
soon loom is landing with more complexity in this area.

Due to the large number of corner cases, there has always been bugs 
hidden in this area. So what I am doing now is to identify odd 
exceptions to our "model" and remove them, to make the code more robust.

Thanks,
/Erik

On 2019-02-14 22:11, Tom Rodriguez wrote:
>
>
> Doug Simon wrote on 2/14/19 12:11 PM:
>> Hi Eric,
>>
>> This seems sensible to me and I cannot recall any special reason it 
>> wasn’t done this way to begin with. Tom, do you have any 
>> recollections about this?
>
> It was originally plugged into the nmethod::oops_do machinery so it 
> happened earlier and I think when we converted to using jweak we kept 
> the existing structure to release those resources more promptly.  The 
> installed_code reference in particular often becomes null very early 
> so releasing it earlier seemed like better hygiene.  I don't think it 
> changes the way anything behaves to do it later but there will be more 
> jweaks apparently alive.
>
> I noticed the assert was removed from the clear methods.  I would 
> prefer to either update them on the CodeCache_lock to match their 
> single caller or simply inline them directly into flush.
>
> tom
>
>
>>
>> -Doug
>>
>>> On 14 Feb 2019, at 12:08, Erik Österlund <erik.osterlund at oracle.com> 
>>> wrote:
>>>
>>> Hi,
>>>
>>> An nmethod goes from being is_alive() to being !is_alive() and 
>>> eventually being freed in nmethod::flush. Native structures for 
>>> nmethods are freed in nmethod::flush when we free the nmethod. 
>>> Except for a few things, including JVMCI handles, that are freed 
>>> already when becoming !is_alive (and some logic to deal with this 
>>> potentially happening twice for a given nmethod), and finally 
>>> asserting during flush that it was already cleared.
>>>
>>> This enhancement proposes to treat these JVMCI handles like we treat 
>>> any other native structure and simply free it in nmethod::flush, 
>>> when we free the nmethod, so that the nmethod and handles have a 1:1 
>>> mapping for their life cycles.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8218969
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8218969/webrev.00/
>>>
>>> Thanks,
>>> /Erik
>>