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

[Vladimir Kozlov]

Looks good to me too.

Thanks,
Vladimir

On 1/29/19 2:40 AM, Tobias Hartmann wrote:
> 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
>>>