[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