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

[Doug Simon]

Hi Eric,

I applaud your effort to restore sanity to method lifecycle management. I’m currently working on a JVMCI change that will remove HotSpotSpeculationLog references from nmethod that will further simplify things.

-Doug

> On 15 Feb 2019, at 10:37, Erik Österlund <erik.osterlund at oracle.com> wrote:
> 
> 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
>>> 
>