[9] RFR(S): 8068945: Use RBP register as proper frame pointer in JIT compiled code on x86
Zoltán Majó
zoltan.majo at oracle.com
Wed Apr 1 13:38:27 UTC 2015
Hi Vladimir,
On 03/30/2015 11:39 PM, Vladimir Kozlov 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.
I ran all tools you've suggested. JFR and jstack is unaffected, pstack
produces nice stack traces (it did not always do so before).
However, I've encountered a problem with SunStudio: Two asserts fail in
the fastdebug build. Both of them "soft" failures, as neither the VM
nor SunStudio crash with the product build. I worked on the problem
today and have a partial understanding of the issue, but more
investigation is needed to have a patch that preserves the correct
behavior of SunStudio as well.
So that will put this RFR on hold for a while, unfortunately.
Thank you for the feedback and suggestions so far!
Best regards,
Zoltan
>
> 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