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

[Roland Westrelin]

Thanks for reviewing this, Chris.

> This is a very nice cleanup too.
> 
> +   assert(bang_size_in_bytes >= frame_size_in_bytes, "stack bang size incorrect”);
> 
> I’m pretty sure this is almost always true but it might not (for whatever reason).  I don’t see much value in that assert.

With the current code, yes. But what if changes are made to the stack banging code? Wouldn’t it be nice to catch a problem if something goes very wrong?

> src/share/vm/opto/compile.cpp:
> 
> + int Compile::bang_size_in_bytes() const {
> +   int callee_locals = method() != NULL ? method()->max_locals() : 0;
> +   int interpreter_frame_size = _interpreter_frame_size;
> +   return MAX2(interpreter_frame_size, frame_size_in_bytes());
> + }
> 
> callee_locals is unused.  Is there a reason you load _interpreter_frame_size into a local variable?

Thanks for catching that. I’ll clean it up.

> 
> src/share/vm/asm/assembler.cpp:
> 
> !     int bang_end = (StackShadowPages+1)*page_size;
> 
> Why +1?

From my first email:

>> 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.



> 
> src/share/vm/ci/ciMethod.cpp:
> 
> + int ciMethod::get_stack_effect_at_invoke(int bci, Bytecodes::Code code, int& inputs) {
> + int ciMethod::stack_effect_if_at_invoke(int bci) {
> 
> Either both with or without “get”.

Ok.

> Since this is very hard to test what testing did you do?

Indeed.
I logged every stack size computation that the compilers do, ran some tests (some subset of specjvm98) with DeoptimizeALot and verified with a script that the stack size computation at deoptimization matches the one done by the compilers before.
I ran regression tests from: java/lang, java/util, hotspot/compiler, hotspot/runtime, hotspot/gc
+ nsk.stress, vm.compiler, vm.regression, nsk.regression, nsk.monitoring
with -Xcomp and with and without -XX:+DeoptimizeALot on x64.

Roland.

> 
> On Mar 6, 2014, at 3:08 AM, Roland Westrelin <roland.westrelin at oracle.com> 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.
>> 
>> 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.
>> 
>> http://cr.openjdk.java.net/~roland/8032410/webrev.01/
>> 
>> Roland.
>