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

[Erik Österlund]

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.

Thanks,
/Erik

> cheers,
> Per