RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
Dean Long
dean.long at oracle.com
Wed Jun 3 21:56:07 UTC 2020
Hi Serguei, I like the latest changes so that JVMCI matches C2. Please
get another review because this is not a trivial change.
dl
On 6/3/20 10:06 AM, serguei.spitsyn at oracle.com wrote:
> Hi Dean,
>
> The updated webrev is:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.3/
>
> Probably, the JVMCI part can be simplified.
> Only the compile_state line has to be moved up:
> + JVMCICompileState compile_state(task);
> // Skip redefined methods
> - if (target_handle->is_old()) {
> + if (compile_state.target_method_is_old()) {
> failure_reason = "redefined method";
> retry_message = "not retryable";
> compilable = ciEnv::MethodCompilable_never;
> } else {
> - JVMCICompileState compile_state(task);
> Fixes in the jvmciEnv.?pp are not really needed
>
> Please, let me know what do you think.
>
> This version does not fail at all (in 300 runs for both C2 and JVMCI).
> It seems, other two issues disappeared as well:
>
> This was seen with the C2:
> https://bugs.openjdk.java.net/browse/JDK-8245128
>
> This was seen with the JVMCI:
> https://bugs.openjdk.java.net/browse/JDK-8245446
>
> Thanks,
> Serguei
>
>
> On 6/1/20 23:40, serguei.spitsyn at oracle.com wrote:
>> Hi Dean,
>>
>> Thank you for the reply.
>>
>> The problem is I do not fully understand your suggestion, especially
>> the part
>> about caching the method,is_old() value in the cache_jvmti_state().
>>
>> This is a preliminary webrev where I tried to implement your suggestion:
>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/kitchensink-comp.2/
>>
>> This variant is failing in half of test runs for both C1/C2 and JVMCI.
>> I think, the root cause is a safepoint in a ThreadInVMfromNative
>> desctructor.
>> Here:
>> 232 void ciEnv::cache_jvmti_state() {
>> 233 VM_ENTRY_MARK;
>>
>> Then we check for the target_method_is_old() value which is not
>> up-to-date any more.
>> I feel, it was correct and more simple before introducing this approach.
>> Probably, I'm missing something here.
>>
>>
>> I also have a question about the update fragment:
>> 1696 {
>> 1697 // Must switch to native to allocate ci_env
>> 1698 ThreadToNativeFromVM ttn(thread);
>> 1699 ciEnv ci_env((CompileTask*)NULL);
>> 1700
>> 1701 // Switch back to VM state to do compiler initialization
>> 1702 ThreadInVMfromNative tv(thread);
>> 1703 ResetNoHandleMark rnhm;
>> 1704
>> 1705 // Perform per-thread and global initializations
>> 1706 comp->initialize();
>> 1707 }
>> Can we remove the ciEnv object initialization above with the state
>> transitions?
>> Or it has some side effects?
>>
>> Please, let me know what you think.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 6/1/20 15:10, Dean Long wrote:
>>> 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/20200603/026ed517/attachment-0001.htm>
More information about the serviceability-dev
mailing list