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

[Erik Österlund]

Hi Per,

Thanks for the review.

/Erik

On 2019-02-21 13:18, Per Liden wrote:
> 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