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

[serguei.spitsyn at oracle.com]

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 
>>>>
>>