Request for reviews (M): 7090904: JSR 292: JRuby junit test crashes in PSScavengeRootsClosure::do_oop

[Christian Thalinger]

I updated the webrev with all review comments and did another round of testing.  SPARC looks good so far (at least 32-bit; 64-bit doesn't work because of a JRuby bug).

-- Chris

On Oct 19, 2011, at 9:26 AM, Christian Thalinger wrote:

> 
> On Oct 18, 2011, at 11:16 PM, Vladimir Kozlov wrote:
> 
>> Christian Thalinger wrote:
>>>> I think you need move the check inside assert:
>>>> 
>>>> +     if (caller->is_interpreted_frame()) {
>>>> +       assert(locals < caller->fp() + frame::interpreter_frame_initial_sp_offset, "bad placement");
>>>> +     }
>>>> ---
>>>> +     assert(!caller->is_interpreted_frame() ||
>>>> +            (locals < caller->fp() + frame::interpreter_frame_initial_sp_offset), "bad placement");
>>> Why do you think that?  I very easy to read with the if.
>> 
>> I don't trust C++ compilers to eliminate is_interpreted_frame() call in product VM even if the body of 'if' statement is empty. You can enclose it with #ifdef ASSERT if you want to keep 'if'.
> 
> :-)  Well, I would trust them since it's the simplest optimization of all but I'll add an #ifdef ASSERT.  Thanks for the review.
> 
> -- Chris
> 
>> 
>> Vladimir
>> 
>>> -- Chris
>>>> 
>>>> Why assert was not added to sparc?
>>>> 
>>>> You did not explain changes in bytecodeTracer.cpp and in frame.cpp (frame::describe()). They seems unrelated to this fix.
>>>> 
>>>> Thanks,
>>>> Vladimir
>>>> 
>>>> Christian Thalinger wrote:
>>>>> http://cr.openjdk.java.net/~twisti/7090904/
>>>>> 7090904: JSR 292: JRuby junit test crashes in PSScavengeRootsClosure::do_oop
>>>>> Reviewed-by:
>>>>> A couple of JRuby junit tests crash in various forms (mostly GC
>>>>> related) when running on 64-bit Linux.  Although it looks like a GC
>>>>> related problem it actually is a deoptimization bug.
>>>>> Tom already had a fix ready but we were unable to write a test case to
>>>>> prove the correctness of the fix.  An assert added in
>>>>> AbstractInterpreter::layout_activation shows that the JRuby crashes
>>>>> are indeed an incarnation of this deoptimization bug.
>>>>> The bug itself is in the deoptimization logic that calculates the size
>>>>> of parameter space in the caller frame.  Method handle invokes may
>>>>> involve fairly arbitrary chains of calls so it's impossible to know
>>>>> how much actual space the caller has for locals.
>>>>> The fix is to always assume zero parameters for method handle call
>>>>> sites.
>>>>> Extensive testing with JRuby junit tests.
>>>>> src/share/vm/runtime/deoptimization.cpp
>>>>> src/cpu/x86/vm/templateInterpreter_x86_32.cpp
>>>>> src/cpu/x86/vm/templateInterpreter_x86_64.cpp
>>>>> src/share/vm/interpreter/bytecodeTracer.cpp
>>>>> src/share/vm/runtime/frame.cpp
>>>>> src/share/vm/runtime/frame.hpp
>>>>> src/share/vm/runtime/thread.cpp
>