RFR: 8215491: ICStubInterface::finalize finds zombie nmethod with ZGC concurrent class unloading

[Erik Osterlund]

Hi Coleen,

Agreed. Thanks for the review!

/Erik

> On 19 Dec 2018, at 19:48, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
> I think this looks good.  Then we don’t have to know who has already cleaned them or if they did it in the right order. 
> 
> Thanks
> Coleen
> 
>> On Dec 18, 2018, at 4:30 AM, Erik Österlund <erik.osterlund at oracle.com> wrote:
>> 
>> Hi Coleen,
>> 
>>>> On 2018-12-17 22:31, coleen.phillimore at oracle.com wrote:
>>>> On 12/17/18 2:57 PM, Erik Österlund wrote:
>>>> Hi Dean,
>>>> 
>>>>>> On 2018-12-17 20:27, dean.long at oracle.com wrote:
>>>>>> On 12/17/18 8:56 AM, Erik Österlund wrote:
>>>>>> Hi,
>>>>>> 
>>>>>> Today, when converting a not_entrant nmethod to zombie, the sweeper first clears the IC stubs of that nmethod, and then make_zombie(). With concurrent class unloading, this ordering is a bit unfortunate. Between clearing the IC stubs and making it zombie, the concurrent GC threads may hit a timing window where they will create IC stubs for concurrently cleaning the ICs of this still is_alive() nmethod. 
>>>>> 
>>>>> Do we normally have to creates stubs to clean an IC?
>>>> 
>>>> Yes if you clean an IC in an nmethod that is_alive(), we currently have to do that using IC stubs. I have some ideas going forward for how we can stop doing that, but that is outside of scope for now.
>>>> 
>>>>> 
>>>>>> The result is that during safepoint cleanup, when we finalize the IC stubs, we find one associated with a zombie.
>>>>>> 
>>>>>> The unregistering of the nmethod from the GC will block during concurrent nmethod unloading, so clearing the IC stubs after the nmethod has become zombie is a lot more sane as there can not be any such races then.
>>>>>> 
>>>>> 
>>>>> Where is the unregistering happening?
>>>> 
>>>> Inside of make_zombie() we unregister_nmethod() on CollectedHeap. In there, ZGC makes sure we wait until concurrent nmethod unloading is over before continuing.
>>> Can you move clean_ic_stubs() inside of make_not_entrant_or_zombie() to maintain the order dependence?
>> 
>> Sure. The implication of that is that the sweeper will redundantly check for IC stubs to clear on unloaded nmethods that can't have any. But I agree that the model is more clear if it is inside of make_not_entrant_or_zombie(), and think that is worth the cost.
>> 
>> New webrev:
>> http://cr.openjdk.java.net/~eosterlund/8215491/webrev.01/
>> 
>> Thanks for reviewing.
>> 
>> /Erik
>> 
>>> thanks,
>>> Coleen
>>>> 
>>>>> 
>>>>> Doing the clearing after it's a zombie does sound safer.
>>>> 
>>>> Agreed!
>>>> 
>>>> Thanks for reviewing.
>>>> 
>>>> /Erik
>>>> 
>>>>> dl
>>>>> 
>>>>>> Bug:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8215491
>>>>>> 
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~eosterlund/8215491/webrev.00/index.html
>>>>>> 
>>>>>> Thanks,
>>>>>> /Erik
>>>>> 
>