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

[Dean Long]

On 5/31/20 11:16 PM, serguei.spitsyn at oracle.com wrote:
> Hi Dean,
>
> To check the is_old as you suggest the target method has to be passed
> to the cache_jvmti_state() as argument. Is it what you are suggesting?

I believe you can use use _task->method()->is_old(), as the ciEnv 
already has the task.

> Just want to make sure I understand you correctly.
>
> The cache_jvmti_state() and cache_dtrace_flags() are called in the
> CompileBroker::init_compiler_runtime() for a ciEnv with the NULL 
> CompileTask
> which looks unnecessary (or I don't understand it):
>
> bool CompileBroker::init_compiler_runtime() {
>   CompilerThread* thread = CompilerThread::current();
>   . . .
>     ciEnv ci_env((CompileTask*)NULL);
>     // Cache Jvmti state
>     ci_env.cache_jvmti_state();
>     // Cache DTrace flags
>     ci_env.cache_dtrace_flags();
>

These calls look unnecessary to me, as the ci_env will cache these again 
before compiling a method.
I suggest removing these calls.  We should make sure the cache fields 
are initialized to sane values
in the ciEnv ctor.

> The JVMCI has a separate implementation for ciEnv which is jvmciEnv and
> its own set of cache_jvmti_state() and jvmti_state_changed() functions.
> Both are not called in the JVMCI case.
> So, these checks look as broken in JVMCI now.
>
JVMCI is in better shape, because it doesn't transition out of 
_thread_in_vm state,
but yes it needs similar changes.

> Not sure, I have enough compiler knowledge to fix this at this stage 
> of release.
> Would it better to file a separate hotspot/compiler RFE targeted to 16?
> It can be assigned to me if it helps.
>

This is a P3 so I believe we have time to fix it for 15.  Please go 
ahead and let's see if
we can get it in.  I can help with the JVMCI changes if they are not 
straightforward.

dl

> Thanks,
> Serguei
>
>
> On 5/28/20 10:54, Dean Long wrote:
>> 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
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200601/ff2f86aa/attachment.htm>