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

Vladimir Kozlov vladimir.kozlov at oracle.com
Wed Apr 22 21:09:54 UTC 2015


You can always find a test which will regress with any changes :)
We need to enable the flag by default for now to test new code path.
As I said we can switch it off later - it may be not final value which 
will be shipped.

Vladimir

On 4/22/15 11:42 AM, Vitaly Davidovich wrote:
> Vladimir,
>
> I don't think preserving frame pointer should default to true.  I
> realize you're relying on performance group to detect regressions, but
> how confident are you that they cover every conceivable scenario?
> Personally, I'd rather this flag be off by default (keep that register
> allocatable) as most folks won't be running linux perf or the like
> regularly, and if they want nice call stacks, they can opt in.
>
> What do you think?
>
> sent from my phone
>
> On Mar 30, 2015 5:39 PM, "Vladimir Kozlov" <vladimir.kozlov at oracle.com
> <mailto:vladimir.kozlov at oracle.com>> wrote:
>
>     On 3/30/15 2:13 PM, Zoltán Majó wrote:
>
>         Hi Vladimir,
>
>
>         thank you for the feedback!
>
>         On 03/30/2015 10:27 PM, Vladimir Kozlov wrote:
>
>             How about PreserveFramePointer instead of simple FramePointer?
>
>             PreserveFramePointer will mean that compiled (or other) code
>             will use
>             that register only as Frame pointer.
>
>
>         I will change the flag's name to PreserveFramePointer and will also
>         update the description.
>
>             Zoltan, x86 flags setting should be in general
>             globals_x86.hpp. You
>             can #ifdef _LP64 there too. I don't understand why you only
>             set it to
>             true on linux-x64.
>
>
>         I remembered that the original discussion with Brendan Gregg
>         mentioned
>         only Linux's perf tool as a possible use case for "proper" frame
>         pointers. So I was unsure whether to enable proper frame pointers by
>         default on other x64 platforms as well.
>
>         But if you think it would be better to have proper frame
>         pointers on all
>         x64 platforms, I will change the code to set PreserveFramePointer to
>         true for all x64 platforms. Just please let me know.
>
>
>     Currently compiled code for all x86 platforms is almost the same
>     (win64 has difference in registers usage) and we should keep it that
>     way.
>
>     Also the original request was to have flag to enable such behavior
>     (use RBP only as FP). So to have it off by default is acceptable. If
>     performance group or someone find a regression (or bug) due to this
>     change we can switch the flag off by default before jdk9 release.
>
>     Try to run pstack on Solaris and jstack on OSX to make sure they
>     report correct call stack with compiled java methods. And JFR.
>     Also it would be nice to run SunStudio analyzer to verify that it works.
>
>     Thanks,
>     Vladimir
>
>
>         Thank you!
>
>         Best regards,
>
>
>         Zoltan
>
>
>             Thanks,
>             Vladimir
>
>             On 3/30/15 8:30 AM, Zoltán Majó wrote:
>
>                 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.
>
>
>
>
>


More information about the hotspot-compiler-dev mailing list