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

[Christian Hagedorn]

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