RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Mon Jun 8 08:59:25 UTC 2020
Thank you a lot for review, Christian!
Serguei
On 6/8/20 00:34, Christian Hagedorn wrote:
> Hi Serguei
>
> Thanks for fixing this. I don't have official reviewer status but the
> changes look good to me.
>
> As we've already discussed, this does not fix JDK-8245128, unfortunately.
>
> Best regards,
> Christian
>
> On 04.06.20 01:05, serguei.spitsyn at oracle.com wrote:
>> Hi Dean,
>>
>> Thank you a lot for the review!
>> I hope, Christian will have a chance to look at it.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 6/3/20 14:56, Dean Long wrote:
>>> 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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
More information about the serviceability-dev
mailing list