Move pushMappingToContainer to RenderBoxModelObject

Move pushMappingToContainer to RenderBoxModelObject

RenderBox and RenderInline currently implement almost identical
pushMappingToContainer methods. Merge the two and move the
implementation to their shared parent; RenderBoxModelObject.

BUG=251019
TEST=No new tests, no change in functionality
R=esprehn@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152631

[esprehn, 2013-06-17 23:56:44]

LGTM, but the perf is uncertain.

I'm a little concerned since this adds 2 virtual calls to a place where there
were none before. Can you use parent()->childrenInline() to your advantage
somehow?

https://codereview.chromium.org/17327003/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/17327003/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBoxModelObject.cpp:2677: bool isFixedPos =
!isRenderInline() && style()->position() == FixedPosition;
isRenderInline is virtual, and now you call it twice in places were we never
called it before. Is this okay for perf? Do we want to cache the value?

[eae, 2013-06-18 00:06:50]

On 2013/06/17 23:56:44, esprehn wrote:
> LGTM, but the perf is uncertain.
> 
> I'm a little concerned since this adds 2 virtual calls to a place where there
> were none before. Can you use parent()->childrenInline() to your advantage
> somehow?

As was I originally but as far as I can tell pushMappingToContainer is only ever
called through a RenderObject* pointer, thus it is always virtual.

[eae, 2013-06-18 00:06:59]

https://codereview.chromium.org/17327003/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/17327003/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBoxModelObject.cpp:2677: bool isFixedPos =
!isRenderInline() && style()->position() == FixedPosition;
On 2013/06/17 23:56:45, esprehn wrote:
> isRenderInline is virtual, and now you call it twice in places were we never
> called it before. Is this okay for perf? Do we want to cache the value?

Both isRenderInline() and style() are marked as const so the compiler should be
smart enough to realize it is unchanged. I'll gladly change it though to cache
the value if you think it'll help though.

[esprehn, 2013-06-18 00:12:38]

On 2013/06/18 00:06:59, eae wrote:
>
https://codereview.chromium.org/17327003/diff/1/Source/core/rendering/RenderB...
> File Source/core/rendering/RenderBoxModelObject.cpp (right):
> 
>
https://codereview.chromium.org/17327003/diff/1/Source/core/rendering/RenderB...
> Source/core/rendering/RenderBoxModelObject.cpp:2677: bool isFixedPos =
> !isRenderInline() && style()->position() == FixedPosition;
> On 2013/06/17 23:56:45, esprehn wrote:
> > isRenderInline is virtual, and now you call it twice in places were we never
> > called it before. Is this okay for perf? Do we want to cache the value?
> 
> Both isRenderInline() and style() are marked as const so the compiler should
be
> smart enough to realize it is unchanged. I'll gladly change it though to cache
> the value if you think it'll help though.

Hmm, is the compiler that smart? I'm not sure. The method is virtual and const,
but any implementation of it could either const_cast away the constness of the
self pointer or use a mutable var, so I'm not sure if the whole program
optimization is smart enough for that.

I'll defer to whatever you think makes sense.

[esprehn, 2013-06-18 00:14:50]

On 2013/06/18 00:06:50, eae wrote:
> ... 
> > I'm a little concerned since this adds 2 virtual calls to a place where
there
> > were none before. Can you use parent()->childrenInline() to your advantage
> > somehow?
> 
> As was I originally but as far as I can tell pushMappingToContainer is only
ever
> called through a RenderObject* pointer, thus it is always virtual.

Yeah, but previously it was 1 virtual call and now it's 2 virtual calls with one
to pushMappingToContainer and another to isRenderInline (maybe 2 depending on
your compiler). This method is in the middle of hit testing which is pretty hot,
especially on pages with a lot of layers.

If you're confident about the patch lets do it though!

[eae, 2013-06-18 00:19:27]

On 2013/06/18 00:14:50, esprehn wrote:
> On 2013/06/18 00:06:50, eae wrote:
> > ... 
> > > I'm a little concerned since this adds 2 virtual calls to a place where
> there
> > > were none before. Can you use parent()->childrenInline() to your advantage
> > > somehow?
> > 
> > As was I originally but as far as I can tell pushMappingToContainer is only
> ever
> > called through a RenderObject* pointer, thus it is always virtual.
> 
> Yeah, but previously it was 1 virtual call and now it's 2 virtual calls with
one
> to pushMappingToContainer and another to isRenderInline (maybe 2 depending on
> your compiler). This method is in the middle of hit testing which is pretty
hot,
> especially on pages with a lot of layers.
> 
> If you're confident about the patch lets do it though!

I'll change it to cache the value. Having copy-and-pasted code seems much worse
(and error prone).

If performance turns out to be an issue we could probably eliminate some of the
branching by using bitwise logic instead.

[esprehn, 2013-06-18 00:20:06]

On 2013/06/18 00:19:27, eae wrote:
>...
> > If you're confident about the patch lets do it though!
> 
> I'll change it to cache the value. Having copy-and-pasted code seems much
worse
> (and error prone).
> 
> If performance turns out to be an issue we could probably eliminate some of
the
> branching by using bitwise logic instead.

SGTM.

[commit-bot: I haz the power, 2013-06-18 00:58:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eae@chromium.org/17327003/10001

[commit-bot: I haz the power, 2013-06-18 03:18:47]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-18 14:01:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eae@chromium.org/17327003/10001

[commit-bot: I haz the power, 2013-06-18 14:10:22]

Change committed as 152631