[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
Mon Mar 30 21:13:18 UTC 2015
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.
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