RFR: 8218974: Free GC native structures in nmethod::flush

[Per Liden]

On 2/18/19 9:11 AM, Erik Österlund wrote:
> Hi Per,
> 
> On 2019-02-18 07:44, Per Liden wrote:
>> Hi Erik,
>>
>> On 02/14/2019 12:55 PM, Erik Österlund 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 GC data. This enhancement proposes to fix 
>>> that to make the life cycle of nmethods and their native data more 
>>> intuitive.
>>>
>>> In particular ZGC has per-nmethod data. The data is removed when 
>>> unlinking nmethods, as opposed to when they are deleted. This is a 
>>> bit awkward and makes things more difficult than they need to be. 
>>> This patch adds a new CollectedHeap::flush_nmethod() function. In 
>>> there ZGC deletes its attached GC data.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8218974
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8218974/webrev.00/
>>
>> Do we need to introduce a new flush_nmethod()? Would it instead be 
>> possible to move/adjust where unregister_nmethod() is called to get 
>> the same effect? When just looking at the API, the relationship 
>> between unregister and flush is not super obvious. Determining which 
>> one will be called first and what a GC allowed/supposed to do in each 
>> of them kind of requires you to inspect the call-sites.
> 
> I think of it this way: unregister_nmethod is tied to the lifecycle of 
> the nmethod oops, and flush_nmethod is for the nmethod itself.
> In particular, we call unregister_nmethod when an nmethod dies (becomes 
> !is_alive()). When an nmethod has died, the oops should not be retained. 
> In fact, when the nmethod becomes unloaded, it dies specifically because 
> the oops are dead, forcing us to kill the nmethod. Then we unregister to 
> tell the GC not to look at those oops again.
> 
> If we moved unregister_nmethod to nmethod::flush, we would keep around 
> nmethods with broken oops in GC data structures, and the GC could no 
> longer trust those data structures, unless we rewrote them to take into 
> consideration that the oops they maintain could be dead if the host 
> nmethod has silently died. But I don't think that would be an improvement.
> 
> Because of this, I think it is wise to separate between GC events for 
> the nmethod dying, and being deleted, because they have different 
> implications.

I hear you. I like how this simplifies the nmethod data life cycle. Just 
one minor thing, ZNMethodTable::lock_for_nmethod() could now be just:

   return gc_data(nm)->lock();

Other than that, look good.

We might want to think about how/if this relates to the BarrierSet::on_* 
functions, with regards to naming and where they live. But that's a 
separate patch.

cheers,
Per