RFR(L): 8032410: compiler/uncommontrap/TestStackBangRbp.java times out on Solaris-Sparc V9

[Vladimir Kozlov]

On 3/11/14 9:35 AM, Roland Westrelin wrote:
>
> Hi Vladimir,
>
>> Changes are good in general.
>
> Thanks for reviewing this.
>
>> I don't see corresponding changes in MachPrologNode in
>> src/cpu/ppc/vm/ppc.ad. Do we need changes there?
>
> We must, I guess. I forgot about c2 ppc. I'll look into it.
>
>> src/share/vm/opto/output.cpp should be
>>
>> +            DEBUG_ONLY(|| true)));
>
> Indeed. Thanks for spotting that.
>
>> On 3/6/14 3:08 AM, Roland Westrelin wrote:
>>> This test causes a deadlock because when the stack bang in the deopt
>>> or uncommon trap blobs triggers an exception, we throw the exception
>>> right away even if the deoptee has some monitors locked. We had
>>> several issues recently with the stack banging in the deopt/uncommon
>>> trap blobs and so rather than add more code to fix stack banging on
>>> deoptimization, this change removes the need for stack banging on
>>> deoptimization as discussed previously:
>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2014-February/013519.html
>>>
>>> The compilers compute by how much deoptimization would bang the stack
>>> at every possible deoptimization points in the compiled code and use
>>> the worst case to generate the stack banging in the nmethod. In debug
>>> builds, the stack banging code is still performed in the
>>> deopt/uncommon trap blobs but only to verify that the compiled code
>>> has done the stack banging correctly. Otherwise, the stack banging
>>> from deoptimization causes the VM to abort.
>>>
>>> This change contains some code
>>> refactoring. AbstractInterpreter::size_activation() is currently
>>> implemented as a call to AbstractInterpreter::layout_activation() but
>>> on most platforms, the logic to do the actual lay out of the
>>> activation and the logic to calculate its size are largely
>>> independent and having both done by layout_activation() feels wrong
>>> to me and error prone. I made AbstractInterpreter::size_activation()
>>> and AbstractInterpreter::layout_activation() two independent methods
>>> that share common helper functions if some code needs to be shared. I
>>> dropped unnecessary arguments to size_activation() in the current
>>> implementation as well. I also made it a template method so that it
>>> can be called with either a Method* (from the deoptimization code) or
>>> a ciMethod* (from the compilers).
>>>
>>> I created src/cpu/x86/vm/templateInterpreter_x86.cpp to put code that is common to 32 and 64 bit x86.

In templateInterpreter_x86.cpp can you add {} for if statement?:

  100 #ifdef ASSERT
  101   if (!EnableInvokeDynamic)


>>>
>>> This change in AbstractAssembler::generate_stack_overflow_check():
>>>
>>> 137     int bang_end = (StackShadowPages+1)*page_size;
>>>
>>> is so that the stack banging code from the deopt/uncommon trap blobs
>>> and in the compiled code are consistent. Let’s say frame size is less
>>> than 1 page. During deoptimization, we bang sp+1 page and then sp+2
>>> pages … sp+(StackShadowPages+1) pages. If the frame size is > 1 page
>>> and <= 2 pages, we bang sp+1page, sp+2pages and then sp+3pages …
>>> sp+(ShadowPages+2) pages. In the compiled code, to be consistent, for
>>> a frame size of less than 1 page we need to bang
>>> sp+(StackShadowPages+1) pages, if it’s > 1 page and <= 2 pages then
>>> we need to bang sp+(StackShadowPages+2) pages etc.
>>
>> With +1 you will touch yellow page because it is inclusive if I read it
>> right:
>>
>>     while (bang_offset <= bang_end) {
>>
>> Can you test with StackShadowPages=1?
>
> Are you suggesting I run with StackShadowPages=1 to check if:
>
> 137     int bang_end = (StackShadowPages+1)*page_size;
>
> is ok?

Yes, because you may be creating hole in banging if compiled code called 
from interpreter. It should be consistent with 
AbstractInterpreterGenerator::bang_stack_shadow_pages().

Should you also change it in generate_native_wrapper()?

> What would I run with StackShadowPages=1? The hotspot-comp regression
> tests? All testing?

I think compiler regression tests with -XX:+DeoptimizeALot flag should 
be enough.

Thanks,
Vladimir

>
> Do you agree that if I revert to:
>
> 137     int bang_end = StackShadowPages*page_size;
>
> I need to change stack banging in the deopt/uncommon trap blobs to bang
> one less page?
>
> Roland.
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> http://cr.openjdk.java.net/~roland/8032410/webrev.01/
>>>
>>> Roland.
>>>