8216541: CompiledICHolders of VM locked unloaded nmethods are released too late

[Tobias Hartmann]

Hi Erik,

okay, got it. Thanks for the details, your fix looks good to me!

Best regards,
Tobias

On 29.01.19 11:38, Erik Österlund wrote:
> Hi Tobias,
> 
> Thanks for having a look at this.
> 
> On 2019-01-29 09:16, Tobias Hartmann wrote:
>> Hi Erik,
>>
>> very nice analysis, thanks a lot for investigating!
>>
>> On 28.01.19 14:56, Erik Österlund wrote:
>>> http://cr.openjdk.java.net/~eosterlund/8216541/webrev.00/
>>
>> Why did you remove the call to thread->set_scanned_compiled_method(NULL) in sweeper.cpp?
> 
> Because the CompiledMethodMarker destructor already nulls this out, and redundantly nulling it out
> again offers no extra protection.
> 
> The idea of nulling it out before calling flush seems to have been to prevent the GC scanning from
> seeing this flushed nmethod in a safepoint, accidentally resurrecting it from the dead. But that is
> already impossible, because flush() is called with a never safepoint checking lock (which guarantees
> we don't have any and can't add any safepoint checks while holding that lock or we will deadlock
> badly). Therefore such safepoints will happen strictly after the processing of the compiled method
> is finished, and it is already cleared the normal way.
> 
> By removing that pointless clearing, I could get rid of the release_compiled_method() function and
> just call flush directly instead. I get confused by there being two "destroy" functions, one in the
> sweeper and one in the nmethod, so I wanted it gone.
> 
>>
>>> The proposed change has survived 200 rounds of kitchensink, hs-tier1-3 and hs-precheckin-comp.
>>
>> In the meanwhile, could you please run some more 100x iterations of kitchensink?
> 
> Sure, running some more as we speak.
> 
> Thanks,
> /Erik
> 
>>
>> Thanks,
>> Tobias
>>