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

[erik.osterlund at oracle.com]

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