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

[Erik Österlund]

Hi,

Rebase over Stefan Karlssons recent ZGC cleanups:
http://cr.openjdk.java.net/~eosterlund/8218974/webrev.01/

Thanks,
/Erik

On 2019-02-21 13:20, Erik Österlund wrote:
> 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
>