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

[Edward Nevill]

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

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.

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.

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

I will prepare a mirror patch for aarch64.

All the best,
Ed.