Request for review (M): 7102657: JSR 292: C1 deoptimizes unlinked invokedynamic call sites infinitely

[Christian Thalinger]

On Oct 21, 2011, at 8:18 PM, Tom Rodriguez wrote:

> 
> On Oct 21, 2011, at 1:22 AM, Christian Thalinger wrote:
> 
>> 
>> On Oct 20, 2011, at 6:52 PM, Tom Rodriguez wrote:
>> 
>>> 
>>> On Oct 20, 2011, at 5:32 AM, Christian Thalinger wrote:
>>> 
>>>> http://cr.openjdk.java.net/~twisti/7102657/
>>>> 
>>>> 7102657: JSR 292: C1 deoptimizes unlinked invokedynamic call sites infinitely
>>>> Reviewed-by:
>>>> 
>>>> The current logic for invokedynamic in LIRGenerator::do_Invoke doesn't
>>>> actually initiate a deoptimization.  We need some new logic to do
>>>> that.
>>> 
>>> c1_Runtime.cpp:
>>> 
>>> This comment isn't true:
>>> 
>>> +   // It's possible the nmethod was invalidated in the last
>>> +   // safepoint, but if it's still alive then make it not_entrant.
>>> 
>>> You've just come from the compiled code so it's required that the caller exists.  Can you change this test to an assert?
>> 
>> Done.
>> 
>>> 
>>> Also I know you copied this comment:
>>> 
>>> +   // Bypass VM_DeoptimizeFrame and deoptimize the frame directly.
>>> 
>>> but it's no really right in any place it's used.  It corresponds to the old code where you could pick how to call the deopt code.  I modified it a while back so that there's only one way and that interface chooses how to call into the deoptimization code.  Can you just correct it to "// Deoptimize the caller frame".
>> 
>> I also changed the comment from where I copied it.
>> 
>>> 
>>> In the stubs, you don't need to explicit logic to dispatch to the deopt blob.  The normal return generated by ~StubFrame will return to the deopt blob because the deoptimize call has rewritten the return address.  That's how all stubs that get deoptimized end up returning to the deopt blob.
>> 
>> Hmm.  I changed the stub to return instead of dispatching to the deopt blob and that produces wrong results with my test case.  Are you sure the return address gets rewritten correctly?
> 
> I didn't notice that you were explicitly requesting unpack_with_reexecution.  That's why you need to dispatch it directly.  Sorry for misleading you.  It would actually work correctly if we took advantage of the reexecute bit in the PcDesc but that would require tweaking the debug info generation process for C1 since we currently don't use it.  You'd have to add a should_reexecute field in CodeEmitInfo and propagate that into IRScopeDebugInfo, so that it can be picked up when actually building the PcDesc.  Anyway, we can keep your original code if you want.

I think it would be nice to use the reexecute bit but I'd like to postpone this to another CR.  Thanks for the review.

-- Chris

> 
> tom
> 
>> 
>>> 
>>> Otherwise it looks good.
>> 
>> Thanks.
>> 
>> -- Chris
>> 
>>> 
>>> tom
>>> 
>>>> 
>>>> src/share/vm/c1/c1_Runtime1.cpp
>>>> src/share/vm/c1/c1_Runtime1.hpp
>>>> src/cpu/sparc/vm/c1_CodeStubs_sparc.cpp
>>>> src/cpu/sparc/vm/c1_Runtime1_sparc.cpp
>>>> src/cpu/x86/vm/c1_CodeStubs_x86.cpp
>>>> src/cpu/x86/vm/c1_Runtime1_x86.cpp
>>>> 
>>> 
>> 
>