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

[Dean Long]

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
>