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

[Roland Westrelin]

Hi Goetz,

> Thanks for considering ppc!  I did the remaining required changes, but I still get 
> some errors.  I'm working on it to get it resolved as soon as possible, and then 
> send the patch to you.

Thanks for working on it. And making the suggestions below!

> I think it's a good idea to refactor layout_activation().  While reading the code I
> saw that you can further simplify the code:
> *  You can remove extra_locals in size_activation() on x86.  It's never used.

Right. Thanks.

> * size_activation_helper on sparc must not use template M.  (ci)Method is not used there.

I had problems with the solaris C compiler that required some static functions to be template functions eventhough the template parameter was not used.

> * After this, only max_locals() is used from M.  I would propose to pass in max_locals()
>   and get rid of the template argument.

Isn’t it only max_stack()? That’s a good suggestion. I will do that.

Roland.

> 
> Best regards,
>  Goetz.
> 
> 
> 
> 
> -----Original Message-----
> From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of Vladimir Kozlov
> Sent: Mittwoch, 12. März 2014 01:27
> To: Roland Westrelin; hotspot-compiler-dev at openjdk.java.net
> Cc: ppc-aix-port-dev at openjdk.java.net
> Subject: Re: RFR(L): 8032410: compiler/uncommontrap/TestStackBangRbp.java times out on Solaris-Sparc V9
> 
> 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.
>>>>