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

[Erik Österlund]

Hi Per,

Thanks for the review.

/Erik

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