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

[Erik Österlund]

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
>