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

[Per Liden]

Looks good!

/Per

On 2/25/19 12:46 PM, Erik Österlund wrote:
> 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
>>
>