[9] RFR(S): 8068945: Use RBP register as proper frame pointer in JIT compiled code on x86

[Zoltán Majó]

Hi Ed,


thank you for your feedback! Please see comments below.

On 03/30/2015 11:11 AM, Edward Nevill wrote:
> Hi Zoltán,
>
> On Fri, 2015-03-27 at 15:34 +0100, Zoltán Majó wrote:
>> Full JPRT run, all tests pass. I also ran all hotspot compiler tests and
>> the jdk tests in java/lang/invoke on both x86_64 and x86_32. All tests
>> that pass without the patch pass also with the patch.
>>
>> I ran the SPEC JVM 2008 benchmarks on our performance infrastructure for
>> x86_64. The performance evaluation suggests that there is no
>> statistically significant performance degradation due to having proper
>> frame pointers. Therefore I propose to have OmitFramePointer set to
>> false by default on x86_64 (and set to true on all other platforms).
> This patch looks good, however I think there is a problem with the logic of OmitFramePointer.
>
> Here is my test case.
>
> --- CUT HERE ---
> // $Id: fibo.java,v 1.2 2000/12/24 19:10:50 doug Exp $
> // http://www.bagley.org/~doug/shootout/
>
> public class fibo {
>      public static void main(String args[]) {
> 	int N = Integer.parseInt(args[0]);
> 	System.out.println(fib(N));
>      }
>      public static int fib(int n) {
> 	if (n < 2) return(1);
> 	return( fib(n-2) + fib(n-1) );
>      }
> }
> --- CUT HERE ---
>
> If I run it as follows on my x86 64 bit linux.
>
> /work/images/jdk/bin/java -XX:-TieredCompilation -XX:+PrintCompilation -XX:CompileOnly=fibo::fib -XX:+UnlockDiagnosticVMOptions -XX:-OmitFramePointer -XX:+PrintAssembly fibo 43
>
> I get
>
>    # {method} {0x00007fc62c97f388} 'fib' '(I)I' in 'fibo'
>    # parm0:    rsi       = int
>    #           [sp+0x30]  (sp of caller)
>    0x00007fc625071100: mov    %eax,-0x14000(%rsp)
>    0x00007fc625071107: push   %rbp
>    0x00007fc625071108: mov    %rsp,%rbp
>    0x00007f836907110b: sub    $0x20,%rsp         ;*synchronization entry
>
> which is correct, it is NOT(-) OmitFramePointer, therefore it is using the frame pointer
>
> Now if I try just changing -XX:-OmitFramePointer to -XX:+OmitFramePointer in the above I get
>
> /work/images/jdk/bin/java -XX:-TieredCompilation -XX:+PrintCompilation -XX:CompileOnly=fibo::fib -XX:+UnlockDiagnosticVMOptions -XX:+OmitFramePointer -XX:+PrintAssembly fibo 43
>
> I get
>
>    # {method} {0x00007f14d3c00388} 'fib' '(I)I' in 'fibo'
>    # parm0:    rsi       = int
>    #           [sp+0x30]  (sp of caller)
>    0x00007f14e1071100: mov    %eax,-0x14000(%rsp)
>    0x00007f14e1071107: push   %rbp
>    0x00007f14e1071108: sub    $0x20,%rsp         ;*synchronization entry
>
> which is correct, it is ID(+) OmitFramePointer, therefore it does not use a frame pointer.
>
> However, if I now delete the -XX:+/-OmitFramePointer altogether, IE
>
> /work/images/jdk/bin/java -XX:-TieredCompilation -XX:+PrintCompilation -XX:CompileOnly=fibo::fib -XX:+UnlockDiagnosticVMOptions -XX:+PrintAssembly fibo 43
>
> I get
>
>    # {method} {0x00007f0c4b730388} 'fib' '(I)I' in 'fibo'
>    # parm0:    rsi       = int
>    #           [sp+0x30]  (sp of caller)
>    0x00007f0c75071100: mov    %eax,-0x14000(%rsp)
>    0x00007f0c75071107: push   %rbp
>    0x00007f0c75071108: sub    $0x20,%rsp         ;*synchronization entry
>
> It is not using a frame pointer which is the equivalent of -XX:+OmitFramePointer. However in your description above you say
>
>> Therefore I propose to have OmitFramePointer set to false by default on x86_64 (and set to true on all other platforms).
> whereas OmitFramePointer actually seems to be set to true on x86_64
>
> I think the problem may be with the declaration and definition of OmitFramePointer in globals.hpp and globals_x86.hpp
>
> In globals.hpp it does
>
> product(bool, OmitFramePointer, true,
>
> In globals_x86.hpp it does
>
> LP64_ONLY(define_pd_global(bool, OmitFramePointer, false););
>
> I am not sure that you can mix product(...) and product_pd(...) like this, so I think it just ends up getting the default from the product(...).

You are right, mixing product and product_pd does not make sense at all. 
Thank you for doing additional testing and for drawing attention to the 
problem.

I updated the code to use product_pd and define_pd_global on all 
relevant platforms.

> Aside: In general, I do not like options which include a negative in them because I have to do a double think when I see something like, -XX:-OmitFramePointer, as in, it is omitting the frame pointer, therefore it is using a frame pointer. How about FramePointer so we have -XX:+FramePointer to say I want frame pointers and -XX:-FramePointer to say I don't.

That is a good idea. Double negation is an unnecessary complication, so 
I changed the name of the flag to FramePointer, just as you suggested.

>
> I did some timing on the above 'fibo' test
>
> [ed at mylittlepony java]$ time /work/images/jdk/bin/java -XX:-OmitFramePointer fibo 43
> 701408733
>
> real	0m1.545s
> user	0m1.571s
> sys	0m0.015s
> [ed at mylittlepony java]$ time /work/images/jdk/bin/java -XX:+OmitFramePointer fibo 43
> 701408733
>
> real	0m1.504s
> user	0m1.527s
> sys	0m0.019s
>
> which is ~3% difference on this test case. On aarch64, I see ~7% difference on this test case.

Thank you for the performance measurements!

> With the above change to fix the logic of OmitFramePointer (and possible change its name) the patch looks good to me.

Here is the updated webrev (the same webrev that was already included 
into my reply to Roland):

http://cr.openjdk.java.net/~zmajo/8068945/webrev.01/

> I will prepare a mirror patch for aarch64.

That would be great!

Thank you and best regards,


Zoltán

>
> All the best,
> Ed.
>
>
>