RFR: 8218974: Free GC native structures in nmethod::flush
erik.osterlund at oracle.com
Thu Feb 21 12:20:28 UTC 2019
Thanks for the review.
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:
>>>> 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.
>>> 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
>> 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
> I hear you. I like how this simplifies the nmethod data life cycle.
> Just one minor thing, ZNMethodTable::lock_for_nmethod() could now be
> 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.
More information about the hotspot-gc-dev