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

[Vladimir Kozlov]

+1

thanks,
Vladimir

On 6/24/20 7:32 AM, Erik Österlund wrote:
> Hi Christian,
> 
> Looks good.
> 
> Thanks,
> /Erik
> 
> On 2020-06-24 15: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 
>>>>>
>>>
>