RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods

[Dean Long]

Sure, you could just have cache_jvmti_state() return a boolean to bail 
out immediately for is_old.

dl

On 5/28/20 7:23 AM, serguei.spitsyn at oracle.com wrote:
> Hi Dean,
>
> Thank you for looking at this!
> Okay. Let me check what cab be done in this direction.
> There is no point to cache is_old. The compilation has to bail out if 
> it is discovered to be true.
>
> Thanks,
> Serguei
>
>
> On 5/28/20 00:59, Dean Long wrote:
>> This seems OK as long as the memory barriers in the thread state 
>> transitions prevent the C++ compiler from doing something like 
>> reading is_old before reading redefinition_count.  I would feel 
>> better if both JVMCI and C1/C2 cached is_old and redefinition_count 
>> at the same time (making sure to be in the _thread_in_vm state), then 
>> bail out based on the cached value of is_old.
>>
>> dl
>>
>> On 5/26/20 12:04 AM, serguei.spitsyn at oracle.com wrote:
>>> On 5/25/20 23:39, serguei.spitsyn at oracle.com wrote:
>>>> Please, review a fix for:
>>>> https://bugs.openjdk.java.net/browse/JDK-8245126
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.1/
>>>>
>>>>
>>>> Summary:
>>>>   The Kitchensink stress test with the Instrumentation module 
>>>> enabled does
>>>>   a lot of class retransformations in parallel with all other 
>>>> stressing.
>>>>   It provokes the assert at the compiled code installation time:
>>>>     assert(!method->is_old()) failed: Should not be installing old 
>>>> methods
>>>>
>>>>   The problem is that the CompileBroker::invoke_compiler_on_method 
>>>> in C2 version
>>>>   (non-JVMCI tiered compilation) is missing the check that exists 
>>>> in the JVMCI
>>>>   part of implementation:
>>>> 2148     // Skip redefined methods
>>>> 2149     if (target_handle->is_old()) {
>>>> 2150       failure_reason = "redefined method";
>>>> 2151       retry_message = "not retryable";
>>>> 2152       compilable = ciEnv::MethodCompilable_never;
>>>> 2153     } else {
>>>> . . .
>>>> 2168     }
>>>>
>>>>    The fix is to add this check.
>>>
>>> Sorry, forgot to explain one thing.
>>> Compiler code has a special mechanism to ensure the JVMTI class 
>>> redefinition did
>>> not happen while the method was compiled, so all the assumptions 
>>> remain correct.
>>>    2190     // Cache Jvmti state
>>>    2191     ci_env.cache_jvmti_state();
>>> Part of this is a check that the value of 
>>> JvmtiExport::redefinition_count() is
>>> cached in ciEnv variable: _jvmti_redefinition_count.
>>> The JvmtiExport::redefinition_count() value change means a class 
>>> redefinition
>>> happened which also implies some of methods may become old.
>>> However, the method being compiled can be already old at the point 
>>> where the
>>> redefinition counter is cached, so the redefinition counter check 
>>> does not help much.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>> Testing:
>>>>    Ran Kitchensink test with the Instrumentation module enabled in mach5
>>>>    multiple times for 100 times. Without the fix the test normally fails
>>>>    a couple of times in 200 runs. It does not fail with the fix anymore.
>>>>    Will also submit hs tiers1-5.
>>>>
>>>> Thanks,
>>>> Serguei
>>>
>>
>