Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11434021:
Always inline GetAlignedPointerFromInternalField (Closed)
|
Created:
8 years ago by abarth-chromium Modified:
8 years ago CC:
v8-dev, Michael Starzinger Base URL:
git://github.com/v8/v8.git@bleeding_edge Visibility:
Public. |
DescriptionAlways inline GetAlignedPointerFromInternalField
On Linux, the compiler isn't smart enough to inline
GetAlignedPointerFromInternalField, so we need to tell it more forcefully.
Patch Set 1 #
Messages
Total messages: 7 (0 generated)
I'm sorry that I don't have exact performance numbers for this patch. My Linux machine isn't very performance stable. Supposedly I can tweak the BIOS to make it more stable, but I haven't done that yet. This change does have a significant effect on the profile, however, so I'm pretty sure it will have an effect on the runtime.
I am not sure that this is the right approach, because only the embedder of v8 can decide what is really important and what not, so giving a hint via the normal "inline" seems to be the right thing to do. Anyway, I can't reproduce the claim that GetAlignedPointerFromInternalField is not inlined in Chrome. The following shell magic doesn't produce any output for me: for i in $(find ~/chromium/src/out/Release -name "*.o"); do nm --demangle --undefined $i | grep --files-with-matches --label=$i v8::Object::GetAlignedPointerFromInternalField ; done I built Chrome the usual way via "make -j32 BUILDTYPE=Release" on an x64 gprecise machine. In fact, I verified that *all* functions marked "inline" are actually inlined. How did you build your Chrome? Perhaps there is a difference in the toolchain etc.? If this is the case we should fix the toolchain or its parameters and avoid any ad-hoc changes in the v8 header.
> Anyway, I can't reproduce the claim that GetAlignedPointerFromInternalField is > not inlined in Chrome. I'll look again in more detail when I'm sitting at my Linux machine again. The profile showed GetAlignedPointerFromInternalField being inlined in some instances, but not in other. I verified that it wasn't inline by looking at the disassembly of one of the hot functions. > In fact, I verified that *all* functions marked "inline" are > actually inlined. All the functions in the V8 header, or all function in the entire product? There are certainly functions in WebKit that we mark inline to get internal linkage that aren't actually inlined. > How did you build your Chrome? Perhaps there is a difference > in the toolchain etc.? I was looking at a binary build with GYP_DEFINES="profiling=1". I need to re-check that build and then check the build without profiling=1. > If this is the case we should fix the toolchain or its > parameters and avoid any ad-hoc changes in the v8 header. I'm open to fixing this in whichever way is appropriate. I would expect all users of V8 to want to inline GetAlignedPointerFromInternalField because (1) it's extremely short and (2) its purpose is to optimize the performance of the fast path.
> I'll look again in more detail when I'm sitting at my Linux machine again. I did a normal (non-profiling) release build of chromium-linux WebKit, and here is the disassembly of WebCore::NodeV8Internal::firstChildAttrGetter, which is one of the hot functions in DOM traversal: == Before patch == (gdb) disas 0x0000000000813a90 Dump of assembler code for function _ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE: => 0x0000000000813a90 <+0>: push %rbp 0x0000000000813a91 <+1>: mov %rsi,%rbp 0x0000000000813a94 <+4>: push %rbx 0x0000000000813a95 <+5>: sub $0x8,%rsp 0x0000000000813a99 <+9>: mov (%rsi),%rdi 0x0000000000813a9c <+12>: mov $0x1,%esi 0x0000000000813aa1 <+17>: sub $0x8,%rdi 0x0000000000813aa5 <+21>: callq 0x464ab0 <_ZN2v86Object34GetAlignedPointerFromInternalFieldEi> 0x0000000000813aaa <+26>: testb $0x2,0x1c(%rax) 0x0000000000813aae <+30>: je 0x813b4b <_ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE+187> 0x0000000000813ab4 <+36>: mov 0x40(%rax),%rbx 0x0000000000813ab8 <+40>: test %rbx,%rbx 0x0000000000813abb <+43>: je 0x813b4b <_ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE+187> 0x0000000000813ac1 <+49>: mov 0x8(%rax),%rax 0x0000000000813ac5 <+53>: mov 0x0(%rbp),%rdx 0x0000000000813ac9 <+57>: test %rax,%rax 0x0000000000813acc <+60>: lea -0x8(%rdx),%rcx 0x0000000000813ad0 <+64>: je 0x813b28 <_ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE+152> 0x0000000000813ad2 <+66>: test %rcx,%rcx 0x0000000000813ad5 <+69>: je 0x813af8 <_ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE+104> 0x0000000000813ad7 <+71>: mov -0x8(%rdx),%rcx 0x0000000000813adb <+75>: cmp %rcx,(%rax) 0x0000000000813ade <+78>: sete %al 0x0000000000813ae1 <+81>: test %al,%al 0x0000000000813ae3 <+83>: je 0x813af8 <_ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE+104> 0x0000000000813ae5 <+85>: mov 0x8(%rbx),%rax 0x0000000000813ae9 <+89>: test %rax,%rax 0x0000000000813aec <+92>: je 0x813b30 <_ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE+160> 0x0000000000813aee <+94>: add $0x8,%rsp 0x0000000000813af2 <+98>: pop %rbx 0x0000000000813af3 <+99>: pop %rbp 0x0000000000813af4 <+100>: retq [...snip...] Notice the callq to _ZN2v86Object34GetAlignedPointerFromInternalFieldEi as basically the first thing that this function does. == After patch == (gdb) disas /m 0x0000000000829060 Dump of assembler code for function _ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE: => 0x0000000000829060 <+0>: push %rbp 0x0000000000829061 <+1>: mov %rsi,%rbp 0x0000000000829064 <+4>: push %rbx 0x0000000000829065 <+5>: sub $0x8,%rsp 0x0000000000829069 <+9>: mov (%rsi),%rdx 0x000000000082906c <+12>: mov -0x8(%rdx),%rax 0x0000000000829070 <+16>: lea -0x8(%rdx),%rdi 0x0000000000829074 <+20>: mov -0x1(%rax),%rcx 0x0000000000829078 <+24>: cmpb $0xaa,0xb(%rcx) 0x000000000082907c <+28>: je 0x829100 <_ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE+160> 0x0000000000829082 <+34>: mov $0x1,%esi 0x0000000000829087 <+39>: callq 0x1314820 <_ZN2v86Object38SlowGetAlignedPointerFromInternalFieldEi> 0x000000000082908c <+44>: testb $0x2,0x1c(%rax) 0x0000000000829090 <+48>: mov 0x0(%rbp),%rdx 0x0000000000829094 <+52>: je 0x82910a <_ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE+170> 0x0000000000829096 <+54>: mov 0x40(%rax),%rbx 0x000000000082909a <+58>: test %rbx,%rbx 0x000000000082909d <+61>: je 0x82910a <_ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE+170> 0x000000000082909f <+63>: mov 0x8(%rax),%rax 0x00000000008290a3 <+67>: lea -0x8(%rdx),%rcx 0x00000000008290a7 <+71>: test %rax,%rax 0x00000000008290aa <+74>: je 0x829120 <_ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE+192> 0x00000000008290ac <+76>: test %rcx,%rcx 0x00000000008290af <+79>: je 0x8290d0 <_ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE+112> 0x00000000008290b1 <+81>: mov (%rcx),%rsi 0x00000000008290b4 <+84>: cmp %rsi,(%rax) 0x00000000008290b7 <+87>: sete %al 0x00000000008290ba <+90>: test %al,%al 0x00000000008290bc <+92>: je 0x8290d0 <_ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE+112> 0x00000000008290be <+94>: mov 0x8(%rbx),%rax 0x00000000008290c2 <+98>: test %rax,%rax 0x00000000008290c5 <+101>: je 0x829130 <_ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE+208> 0x00000000008290c7 <+103>: add $0x8,%rsp 0x00000000008290cb <+107>: pop %rbx 0x00000000008290cc <+108>: pop %rbp 0x00000000008290cd <+109>: retq [...snip...] Notice that after the patch, the callq is to _ZN2v86Object38SlowGetAlignedPointerFromInternalFieldEi rather than _ZN2v86Object34GetAlignedPointerFromInternalFieldEi. Please let me know what you think the best way is to fix this issue.
I finally found out what caused the differences in our observations: The release build uses -fvisibility-inlines-hidden, so GetAlignedPointerFromInternalField is of type "t", not "U" as I expected. :-/ I'd like to force inlining consistently in a different CL then.
Superseded by the more general https://codereview.chromium.org/11411355/, closing...
Message was sent while issue was closed.
Thanks! |
||||||||||||||||||||||||||||||||||||||||||||||
