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

[Tobias Hartmann]

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
>>
>