RFR JDK-8195639: [Graal] nsk/jvmti/PopFrame/popframe009 fail with Graal in -Xcomp

dean.long at oracle.com dean.long at oracle.com
Wed Nov 28 21:43:31 UTC 2018


I guess even with -Xcomp, there are still compiles triggered by profiled 
code, otherwise I don't see why changing policy()->event() helps.

Another problem I see with this change is that it will make the real 
problem harder to reproduce.

dl

On 11/28/18 12:36 PM, Alex Menkov wrote:
> Hi guys,
>
> I was able to reproduce the issue only with Graal + -Xcomp and only 
> with debug build (I tried Graal + -Xint, Graal without Xcomp/Xint, 
> just -Xcomp (i.e. C1 is used)).
>
> --alex
>
> On 11/28/2018 01:27, serguei.spitsyn at oracle.com wrote:
>> On 11/28/18 01:07, dean.long at oracle.com wrote:
>>> OK, reconsidering solution 1, I have a couple more questions:
>>>
>>> The bug report says the problem is reproducible with -Xcomp, but I 
>>> don't see the policy()->event() path taken with -Xcomp. Instead, 
>>> -Xcomp uses CompilationPolicy::compile_if_required, so I don't see 
>>> how Solution 1 fixes the problem with -Xcomp.
>>>
>>> > CompilerRuntime always calls policy()->event with CompLevel == 
>>> CompLevel_aot.
>>>
>>> As far as I can tell, CompLevel_aot is only used when there is 
>>> active AOT code.  Is this an AOT-specific problem?
>>
>> Tracing showed that the CompLevel_aot is always used for JVMCI which 
>> is pretty confusing.
>> Most likely, it is because now the AOT is enabled by default (is it 
>> correct?).
>> I'm not sure it actually has anything to do with the AOT mode.
>>
>>
>>>   I would expect there to be a problem with c1 as well.
>>
>> Not sure about the c1.
>> I hope, Alex could comment on this tomorrow.
>>
>>
>> Thanks,
>> Serguei
>>
>>> dl
>>>
>>> On 11/27/18 3:38 PM, serguei.spitsyn at oracle.com wrote:
>>>> Good summary with a list of solutions below!
>>>>
>>>> On 11/27/18 2:50 PM, dean.long at oracle.com wrote:
>>>>> Let me list the proposed solutions:
>>>>>
>>>>> 1. short-circuit compiles in CompilationPolicy::event in 
>>>>> is_interp_only_mode
>>>>> 2. detect is_interp_only_mode in resolve stubs and force a c2i call
>>>>> 3. don't offer a JVMTI suspend point in resolve stubs (or 
>>>>> JavaCallWrapper)
>>>>>
>>>>> Solution 1 penalizes other threads, and don't prevent other 
>>>>> threads from compiling the method, so while it may help a little, 
>>>>> it's incomplete.
>>>>
>>>> I doubt, the Solution 1 penalizes other threads.
>>>> First, it is an important pretty frequent/common case that allows 
>>>> to save on compilations.
>>>> Second, there is no point to compile methods on a thread executed 
>>>> in interp_only_mode.
>>>> Also, there is no big advantage to compile methods even for other 
>>>> threads
>>>> when an active debugging (e.g. single stepping) is in progress.
>>>> Third, this would make it consistent with what the the interpreter 
>>>> is doing.
>>>>
>>>> So, I think, the Solution 1 is needed independently of other 
>>>> solutions we take.
>>>> My suggestion is to apply it in the JDK-8195639 bug fix.
>>>>
>>>>> Solutions 1 and 2 allow the thread to resume in a different method 
>>>>> (the callee method), causing other problems.
>>>>
>>>> Agreed.
>>>>
>>>>> With Solution 3, the frame we suspend is the same as the frame we 
>>>>> end up in after the resume, so I believe it solves all the problems.
>>>>> IMHO this is the correct solution to pursue.
>>>>
>>>> I agree in general.
>>>> This solution has some risks involved as it impacts a lot of code 
>>>> including platform-independent.
>>>> It is also going to fix the other bugs: JDK-8195635, JDK-8214093, 
>>>> JDK-8207013 (not sure about all of them yet).
>>>>
>>>> My suggestion is to pursue it as the JDK-8195635 fix.
>>>> Could you, please, confirm if works for you?
>>>>
>>>> Also, we have no consensus yet on the Solution 1.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>> dl
>>>>>
>>>>> On 11/27/18 2:19 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Dean,
>>>>>>
>>>>>> You also asked the questions:
>>>>>>
>>>>>> > Doesn't is_interp_only_mode() only apply to the current thread?
>>>>>>
>>>>>> Yes it does.
>>>>>>
>>>>>>
>>>>>> > I don't think it's safe to assume no compiles will happen in 
>>>>>> other threads,
>>>>>> > or that a method call target is not already compiled, because 
>>>>>> as far
>>>>>> > as I can tell, JVMTI only deoptimizes the active frames.
>>>>>>
>>>>>> I agree with it.
>>>>>> However, compilations on the thread with the interp_only_mode 
>>>>>> enabled is the most important case.
>>>>>> Disabling compilations such thread improves testing stability.
>>>>>>
>>>>>> > The bug report describes a case where the caller has been 
>>>>>> deoptimized
>>>>>> > while resolving a call.  I don't see how this fix prevents the 
>>>>>> target
>>>>>> > from being a previously compiled method.
>>>>>> > But we do need to make sure not to call into compiled code, so 
>>>>>> I think
>>>>>> > the fix needs to be in code like 
>>>>>> SharedRuntime::resolve_static_call_C(),
>>>>>> > where it returns get_c2i_entry() if is_interp_only_mode() is true.
>>>>>>
>>>>>> Thank you for making this point.
>>>>>>
>>>>>> It would be nice to attack this as well.
>>>>>> We can try to investigate this approach further.
>>>>>> One problem is that there are more cases like resolve_static_call_C,
>>>>>> for instance, resolve_virtual_call_C and resolve_opt_virtual_call_C.
>>>>>> We may need to fix the same in other places, like 
>>>>>> JavaCallWrapper::JavaCallWrapper.
>>>>>>
>>>>>> We can start from fixing it in the resolve_static_call_C.
>>>>>> If it is successful then continue this effort for other cases as 
>>>>>> well.
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>> On 11/27/18 1:01 PM, Alex Menkov wrote:
>>>>>>> Hi Dean,
>>>>>>>
>>>>>>> Thank you for the feedback and for the comments in Jira.
>>>>>>>
>>>>>>> >> How does this solve the problem of
>>>>>>> >> HotSpotJVMCIRuntime.adjustCompilationLevel being called?
>>>>>>>
>>>>>>> It doesn't. It fixes another issue.
>>>>>>> The test suspend test thread and ensures that top frame is a 
>>>>>>> "fibonachi" method. Then it turns SingleStep on, does PopFrame 
>>>>>>> and resumes the thread.
>>>>>>> The problem is the thread continues execution of compiled code 
>>>>>>> (ignoring SingleStep) and we get SindleStep event only when 
>>>>>>> adjustCompilationLevel method is called (it's interpreted code).
>>>>>>>
>>>>>>> There are several other bugs which are (most likely) caused by 
>>>>>>> suspend during call setup (JDK-8195635, JDK-8214093, JDK-8207013)
>>>>>>>
>>>>>>> --alex
>>>>>>>
>>>>>>> On 11/27/2018 01:39, serguei.spitsyn at oracle.com wrote:
>>>>>>>> Hi Dean,
>>>>>>>>
>>>>>>>> Thank you a lot for looking at this!
>>>>>>>> Just a couple of points from me (it is up to Alex to provide a 
>>>>>>>> full answer).
>>>>>>>>
>>>>>>>>
>>>>>>>> I think, Alex in this RFR missed to tell that we knew about 
>>>>>>>> this issue that an incorrect frame will be popped.
>>>>>>>> But after some discussion we decided to file a separate issue 
>>>>>>>> on this.
>>>>>>>> Alex wanted to create a good stand-alone test case 
>>>>>>>> demonstrating this problem before filing it.
>>>>>>>>
>>>>>>>> Now, as I can see, the JDK-8195635 
>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8195635> looks very 
>>>>>>>> close to a bug that we wanted to file.
>>>>>>>> The only difference is that our scenario includes the 
>>>>>>>> SharedRuntime::resolve_static_call_C instead of the 
>>>>>>>> JavaCallWrapper::JavaCallWrapper as a helper for Java method 
>>>>>>>> invocation.
>>>>>>>> If I understand corrctly (Alex will fix me if needed), the 
>>>>>>>> jtreg test we used to chase this issue did not catch a problem 
>>>>>>>> with the HotSpotJVMCIRuntime.adjustCompilationLevel.
>>>>>>>>
>>>>>>>>
>>>>>>>> The suggested fix was to fix the mismatch in the 
>>>>>>>> TieredThresholdPolicy::even with the comment in the interpreter 
>>>>>>>> code:
>>>>>>>> nmethod* 
>>>>>>>> InterpreterRuntime::frequency_counter_overflow(JavaThread* 
>>>>>>>> thread, address branch_bcp) {
>>>>>>>>      . . .
>>>>>>>>    if (nm != NULL && thread->is_interp_only_mode()) {
>>>>>>>>      // Normally we never get an nm if is_interp_only_mode() is 
>>>>>>>> true, because
>>>>>>>>      // policy()->event has a check for this and won't compile 
>>>>>>>> the method when
>>>>>>>>      // true. However, it's possible for is_interp_only_mode() 
>>>>>>>> to become true
>>>>>>>>      // during the compilation. We don't want to return the nm 
>>>>>>>> in that case
>>>>>>>>      // because we want to continue to execute interpreted.
>>>>>>>>      nm = NULL;
>>>>>>>>    }
>>>>>>>>
>>>>>>>>  > So I think the fix needs to be in code like 
>>>>>>>> SharedRuntime::resolve_static_call_C(),
>>>>>>>>  > where it returns get_c2i_entry() if is_interp_only_mode() is 
>>>>>>>> true.
>>>>>>>>
>>>>>>>> I'm not sure, the adding this condition and returning the 
>>>>>>>> get_c2i_entry() is always correct.
>>>>>>>> We need some help from the Compiler team to make it right.
>>>>>>>>
>>>>>>>> BTW, the interp_only_mode has been enabled only when some 
>>>>>>>> interp_only events are enabled.
>>>>>>>> It is not set by either PopFrame or ForceEarlyReturn.
>>>>>>>> But the popframe009 test enables single stepping, so we wanted 
>>>>>>>> to make this part right.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/26/18 16:05, dean.long at oracle.com wrote:
>>>>>>>>> How does this solve the problem of 
>>>>>>>>> HotSpotJVMCIRuntime.adjustCompilationLevel being called?
>>>>>>>>>
>>>>>>>>> I don't think this fix is the right approach. Doesn't 
>>>>>>>>> is_interp_only_mode() only apply to the current thread? I 
>>>>>>>>> don't think it's safe to assume no compiles will happen in 
>>>>>>>>> other threads, or that a method call target is not already 
>>>>>>>>> compiled, because as far as I can tell, JVMTI only deoptimizes 
>>>>>>>>> the active frames. The bug report describes a case where the 
>>>>>>>>> caller has been deoptimized while resolving a call.  I don't 
>>>>>>>>> see how this fix prevents the target from being a previously 
>>>>>>>>> compiled method.  But we do need to make sure not to call into 
>>>>>>>>> compiled code, so I think the fix needs to be in code like 
>>>>>>>>> SharedRuntime::resolve_static_call_C(), where it returns 
>>>>>>>>> get_c2i_entry() if is_interp_only_mode() is true. However, 
>>>>>>>>> there is still another problem. By allowing JVMTI to suspend 
>>>>>>>>> the thread during call setup, but reporting the frame as still 
>>>>>>>>> in the caller instead of the callee, we confuse JVMTI into 
>>>>>>>>> thinking that execution will resume in the caller instead of 
>>>>>>>>> the callee. We may want to restrict where we offer JVMTI 
>>>>>>>>> suspend points, and not offer a JVMTI suspend point in 
>>>>>>>>> SharedRuntime::resolve_static_call_C and friends at all.
>>>>>>>>>
>>>>>>>>> dl
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 11/26/18 11:14 AM, Alex Menkov wrote:
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> Please review the fix for
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8195639
>>>>>>>>>> webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~amenkov/popframe009/webrev.01/
>>>>>>>>>>
>>>>>>>>>> Description:
>>>>>>>>>> The test suspends a thread, turns on single stepping and then 
>>>>>>>>>> calls PopFrame. SingleStep event is expected as soon as the 
>>>>>>>>>> thread is resumed and PopFrame is processed (so we have call 
>>>>>>>>>> stack with the depth 1 less than it was before PopFrame). 
>>>>>>>>>> Instead SingleStep event is received with much deeper call 
>>>>>>>>>> stack (and PopFrame processes wrong frame).
>>>>>>>>>> Research shown that this is caused by missed deoptimization 
>>>>>>>>>> of the current frame.
>>>>>>>>>> As far as I understand CompilationPolicy::event should handle 
>>>>>>>>>> the case when the thread has is_interp_only_mode() set, but 
>>>>>>>>>> TieredThresholdPolicy::event checks this only then CompLevel 
>>>>>>>>>> is CompLevel_none.
>>>>>>>>>> CompilerRuntime always calls policy()->event with CompLevel 
>>>>>>>>>> == CompLevel_aot.
>>>>>>>>>>
>>>>>>>>>> The fix looks quite simple, but I'd appreciate feedback from 
>>>>>>>>>> runtime and compiler teams as I'm not sure I completely 
>>>>>>>>>> understand all the details of the "PopFrame dance".
>>>>>>>>>>
>>>>>>>>>> --alex
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>



More information about the hotspot-compiler-dev mailing list