[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 18:42:34 UTC 2015
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>
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/610b3778/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list