[15] RFR(XS): 8245128: Kitchensink fails with: assert(destination == (address)-1 || destination == entry) failed: b) MT-unsafe modification of inline cache

[Christian Hagedorn]

Thank you Serguei for your review!

Best regards,
Christian

On 25.06.20 04:58, serguei.spitsyn at oracle.com wrote:
> Hi Christian,
> 
> The fix looks good to me.
> Nice catch from Coleen.
> 
> Thanks,
> Serguei
> 
> 
> On 6/24/20 06:17, Christian Hagedorn wrote:
>> Hi Coleen, hi Vladimir
>>
>> Thank you for your reviews!
>>
>> On 23.06.20 22:04, coleen.phillimore at oracle.com wrote:
>>> I know less about this code than Erik and Vladimir but would you need 
>>> to also test for this condition in the b) case?
>>>
>>>   754 !old_method->method_holder()->is_loader_alive() ||
>>>
>>>
>>> You probably won't get as much racy behavior from class unloading 
>>> though.
>>
>> That's a valid concern. I discussed it with Erik Ö. and he think we 
>> need to add that together with old_method == NULL like in the a) case. 
>> Here is a detailed explanation from Erik:
>>
>> "Concurrent class unloading clears the method, so it can be NULL. 
>> There is a narrow race when we are concurrently unloading nmethods, 
>> while others are loaded. The scenario is that we have an optimized 
>> virtual call, materialized kind of like a static call, assuming that 
>> there is only a single implementor. When the single implementor is 
>> unloaded, the caller nmethod will get deoptimized when the GC gets to 
>> it. But before that happens, we can have calls going through that call 
>> site. If before the call, a second implementor gets loaded, I think it 
>> is possible that the new loaded class gets installed as still the only 
>> subclass (single implementor) of the abstract thing. Then, I don't 
>> think the class loading triggers CHA deoptimization of the caller 
>> nmethod. So I think then you can call through that nmethod to an 
>> alternative callee nmethod, due to a race between class unloading, 
>> class loading, compilation and calling an optimized virtual call."
>>
>> I updated this in a new webrev accordingly:
>> http://cr.openjdk.java.net/~chagedorn/8245128/webrev.01/
>>
>> Best regards,
>> Christian
>>
>>
>>> Otherwise, the change makes sense.
>>> thanks,
>>> Coleen
>>>
>>> On 6/23/20 12:31 PM, Vladimir Kozlov wrote:
>>>> Good.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 6/23/20 6:58 AM, Christian Hagedorn wrote:
>>>>> Hi
>>>>>
>>>>> Please review the following patch:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8245128
>>>>> http://cr.openjdk.java.net/~chagedorn/8245128/webrev.00/
>>>>>
>>>>> The assert is often hit when enabling the Kitchensink 
>>>>> instrumentation module which triggers a lot of class redefinitions. 
>>>>> The problem looks similar to the one fixed in JDK-8225681 [1] for 
>>>>> the other a) MT-unsafe assert. We could be dealing with an old 
>>>>> method which we should also exclude in the second b) MT-unsafe 
>>>>> assert (JDK-8225681 fixed it only for a)). A nice description of 
>>>>> the problem is found in the comment [2] by Erik Ö.
>>>>>
>>>>> Applying this fix, the assert is not hit anymore with repeated 
>>>>> Kitchensink runs with the instrumentation module enabled.
>>>>>
>>>>> Thank you!
>>>>>
>>>>> Best regards,
>>>>> Christian
>>>>>
>>>>>
>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8225681
>>>>> [2] 
>>>>> https://bugs.openjdk.java.net/browse/JDK-8225681?focusedCommentId=14278441&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14278441 
>>>>>
>>>
>