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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Tue Nov 27 23:47:22 UTC 2018 


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.

A correction: I wanted to say "platform-dependent".

Thanks,
Serguei


> 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 serviceability-dev mailing list