RFR(XS): 8245126 Kitchensink fails with: assert(!method->is_old()) failed: Should not be installing old methods
Christian Hagedorn
christian.hagedorn at oracle.com
Mon Jun 8 07:34:12 UTC 2020
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