[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 Erik and Vladimir for reviewing it again!

Best regards,
Christian

On 24.06.20 17:55, Vladimir Kozlov wrote:
> +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 
>>>>>>
>>>>
>>