RFR: 8234426: Sweeper should not CompiledIC::set_to_clean with ICStubs for is_unloading() nmethods

[erik.osterlund at oracle.com]

Hi Tobias,

Thanks for the review!

/Erik

On 12/3/19 1:27 PM, Tobias Hartmann wrote:
> Hi Erik,
>
> right, your assert is better and the comment explaining the details is very helpful.
>
> Looks good to me!
>
> Best regards,
> Tobias
>
> On 03.12.19 10:59, erik.osterlund at oracle.com wrote:
>> Hi Tobias,
>>
>> Thank you for the review.
>> We could indeed add an assert like that. Another possibly better assert is assert(!from->is_zombie()).
>> The thing is that is_unloading is strictly monotonic - it goes from false to true (iff GC kills an
>> oop).
>> Therefore, nmethods that are unloaded are always is_unloading(), so your assert will only ever catch
>> zombies
>> (the only flavour of !is_alive() that can be !is_unloading()). However, the sweeper flips unloaded
>> nmethods to zombies today, and these mysterious zombies are also is_unloading() (because they were once
>> unloaded by the GC before they became zombies). Your assert will still allow such zombies, but they
>> should in fact never have their inline caches cleaned.
>>
>> So in one way, asserting that the CompiledMethod is !is_zombie() is strictly more precise than
>> your assert. But it might also be less obvious to the reader. Unless I put a big comment in there
>> describing what I explained above, which of course I could do. Here is an attempt at that:
>>
>> http://cr.openjdk.java.net/~eosterlund/8234426/webrev.01/
>>
>> Incremental:
>> http://cr.openjdk.java.net/~eosterlund/8234426/webrev.00_01/
>>
>> What do you think?
>>
>> Thanks,
>> /Erik
>>
>> On 12/3/19 9:08 AM, Tobias Hartmann wrote:
>>> Hi Erik,
>>>
>>> this looks good to me too. Thanks to Stefan for the additional details.
>>>
>>> Should we assert(from->is_alive() || from->is_unloading()) for sanity?
>>>
>>> Best regards,
>>> Tobias
>>>
>>> On 02.12.19 18:01, Erik Österlund wrote:
>>>> Hi Stefan,
>>>>
>>>> Thank you for the review. Your analysis is exactly right!
>>>>
>>>> Thanks,
>>>> /Erik
>>>>
>>>> On 2019-12-02 15:45, Stefan Karlsson wrote:
>>>>> Hi Erik,
>>>>>
>>>>> After discussing this with Erik, I think this looks good. Just to recapture some of my questions
>>>>> and thoughts:
>>>>>
>>>>> The nmethod can be in three interesting states when the sweeper executes this code:
>>>>> 1) is_alive && !is_unloading
>>>>> 2) is_alive && is_unloading
>>>>> 3) !is_alive && is_unloading
>>>>>
>>>>> The transition from (2) to (3) happens when the GC is unloading the nmethod concurrently while the
>>>>> sweeper is processing the nmethod. It's only interesting to bring up state (3), because this is
>>>>> the reason why we can't assert that the nmethod is_alive.
>>>>>
>>>>> The interesting transition happens between (1) and (2), and this transition is well-synchronized
>>>>> between the GC and sweeper by means of the synchronization protocol in nmethod::is_unloading.
>>>>>
>>>>> In (1) all oops are good and the nmethod is guaranteed to not be unloaded.
>>>>> In (2) at least one of the oops are bad and the nmethod is on its way to become unloaded
>>>>>
>>>>> The sweeper communicates that it's processing an nmethod by exposing it to the root set of the
>>>>> threads. We are OK with exposing (1) nmethods, but we must make sure we never expose (2) nmethods.
>>>>>
>>>>> The only time the sweeper-processed nmethod is exposed to the root set of the threads is when we
>>>>> safepoint. The only path that can safepoint in set_to_clean(bool in_use) is when we're using
>>>>> ICStubs. However, we don't need to use ICStubs when an nmethod is not "in use". is_unloading
>>>>> nmethods (2) are not "in use", otherwise they would have been found by root scanning and/or entry
>>>>> barriers and would have stayed !is_unloading. Therefore the code was changed to
>>>>> set_to_clean(!is_unloading()).
>>>>>
>>>>> Thanks,
>>>>> StefanK
>>>>>
>>>>> On 2019-11-22 11:49, erik.osterlund at oracle.com wrote:
>>>>>> Hi,
>>>>>>
>>>>>> When the sweeper processes an nmethod, it will clean inline caches if it is_alive().
>>>>>> Today, the cleaning will utilize transitional states (using ICStubs) if the nmethod is_alive(),
>>>>>> which is always true for the sweeper. If it runs out of ICStubs, it might have to safepoint
>>>>>> to refill them. When it does, the currently processed nmethod might be is_unloading().
>>>>>> That is not a problem for the GC per se (safepoint operation fusing with mark end), but it
>>>>>> is a problem for heap walkers that get confused that an nmethod reachable from a thread is
>>>>>> unloading
>>>>>> and hence has dead oops in it. This sweeper nmethod is the *only* nmethod that violates an
>>>>>> invariant that nmethods reachable from threads (Thread::nmethods_do) are not unloading.
>>>>>>
>>>>>> By simply changing the condition to not use ICStubs when the nmethod is_unloading(), we
>>>>>> get this neat invariant, and code gets less confused about this.
>>>>>>
>>>>>> Bug:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8234426
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~eosterlund/8234426/webrev.00/
>>>>>>
>>>>>> Thanks,
>>>>>> /Erik