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

Vitaly Davidovich vitalyd at gmail.com
Wed Apr 22 21:13:18 UTC 2015


I'm ok with that if it makes your testing easier for now, but wouldn't want
that to slip in as default in java 9 :).  But at any rate, if this feature
is going to be supported properly, at some point you'll need to have tests
that run both flavors anyway, irrespective of which is the default, so
perhaps just do that now (create test configs with this explicitly enabled,
and disable it by default).  Anyway, your call.

On Wed, Apr 22, 2015 at 5:09 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com
> wrote:

> 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.
>>
>>
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20150422/9c56886e/attachment-0001.html>


More information about the hotspot-compiler-dev mailing list