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

[Tobias Hartmann]

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
>