WIP: redirect some LayoutBox methods to ng fragments

WIP: redirect some LayoutBox methods to ng fragments

BUG=

[dgrogan, 2017-02-16 03:15:59]

dgrogan@chromium.org changed reviewers:
+ atotic@chromium.org, ikilpatrick@chromium.org

[dgrogan, 2017-02-16 03:16:00]

I'm just getting started, but looks like for every method that paint calls,
we'll have to add a conditional and a stack allocation. 

Will these overwhelm what we're trying to see the effects of, namely pointer
dereferences?

Am I overlooking some things that would let us avoid these extra instructions?

Thoughts?

https://codereview.chromium.org/2695963005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2695963005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.h:324: if
(m_ngPhysicalBoxFragment) {
location() is called during layout before NGFragmentBuilder::ToBoxFragment sets 
LayoutBox::m_ngPhysicalBoxFragment, necessitating this check.

https://codereview.chromium.org/2695963005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_physical_box_fragment.h
(right):

https://codereview.chromium.org/2695963005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_physical_box_fragment.h:48: return
LayoutPoint(offset_.left, offset_.top);
Then we have to stack allocate these objects so that we don't have to change
return types of LayoutBox methods.

[atotic, 2017-02-16 06:43:43]

On 2017/02/16 at 03:16:00, dgrogan wrote:
> I'm just getting started, but looks like for every method that paint calls,
we'll have to add a conditional and a stack allocation. 
> 
> Thoughts?

I have not done any performance work on Blink, or C++ in a decade, so my
opinions are probably worthless. 

> Will these overwhelm what we're trying to see the effects of, namely pointer
dereferences?

I expect pointer deref effect to be barely measurable.

> Am I overlooking some things that would let us avoid these extra instructions?

Conditionals are cheap.
Stack allocation might be optimized to a single copy by return value
optimization?

Side comment: sizeof(NGPhysicalFragment) looks heavy.
I remember some tech talk about splitting it up. That seems useful.

[ikilpatrick, 2017-02-16 18:24:21]

Yeah to NGPhysicalFragment with cbiesinger's work will dramatically reduce the
size.

We may have to:
Change the allocation of the NGPhysicalFragment to reside in the same partition
alloc as LayoutObject, but we'll see once we get some initial perf results.

We should also test on a low end android device with limited memory bandwidth to
fully see the implications, i have an android moto e 2nd gen somewhere....

https://codereview.chromium.org/2695963005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2695963005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.h:221: //   
m_ngPhysicalBoxFragment->setX(x);
yeah so this is what i had in mind, I'd delete m_frameRect and just replace all
the locations that touch it to m_ngPhysicalBoxFragment....

https://codereview.chromium.org/2695963005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_physical_box_fragment.h
(right):

https://codereview.chromium.org/2695963005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_physical_box_fragment.h:48: return
LayoutPoint(offset_.left, offset_.top);
On 2017/02/16 03:16:00, dgrogan wrote:
> Then we have to stack allocate these objects so that we don't have to change
> return types of LayoutBox methods.

This should be fine, I don't think we'll see this show up (the compiler should
compile this mostly away)...

[dgrogan, 2017-02-16 18:54:09]

https://codereview.chromium.org/2695963005/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/2695963005/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.h:221: //   
m_ngPhysicalBoxFragment->setX(x);
On 2017/02/16 18:24:20, ikilpatrick wrote:
> yeah so this is what i had in mind, I'd delete m_frameRect and just replace
all
> the locations that touch it to m_ngPhysicalBoxFragment....

What should I do about layout calling location() (and I'm sure others) before
LayoutBox has a m_ngPhysicalBoxFragment? Start with a placeholder and then have
NGFragmentBuilder replace it?

[ikilpatrick, 2017-02-16 19:22:44]

On 2017/02/16 18:54:09, dgrogan wrote:
>
https://codereview.chromium.org/2695963005/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> 
>
https://codereview.chromium.org/2695963005/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutBox.h:221: //   
> m_ngPhysicalBoxFragment->setX(x);
> On 2017/02/16 18:24:20, ikilpatrick wrote:
> > yeah so this is what i had in mind, I'd delete m_frameRect and just replace
> all
> > the locations that touch it to m_ngPhysicalBoxFragment....
> 
> What should I do about layout calling location() (and I'm sure others) before
> LayoutBox has a m_ngPhysicalBoxFragment? Start with a placeholder and then
have
> NGFragmentBuilder replace it?

Oh sorry, i was imaginging for this initial test that a LayoutBox _always_ has a
PhysicalBoxFragment. so location/setLocation will always succeed. Does that make
sense?

This would be without any layoutng code running.

[dgrogan, 2017-02-17 04:11:19]

Patchset #3 (id:40001) has been deleted

[dgrogan, 2017-02-17 05:00:24]

On 2017/02/16 19:22:44, ikilpatrick wrote:
> On 2017/02/16 18:54:09, dgrogan wrote:
> >
>
https://codereview.chromium.org/2695963005/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/layout/LayoutBox.h (right):
> > 
> >
>
https://codereview.chromium.org/2695963005/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/layout/LayoutBox.h:221: //   
> > m_ngPhysicalBoxFragment->setX(x);
> > On 2017/02/16 18:24:20, ikilpatrick wrote:
> > > yeah so this is what i had in mind, I'd delete m_frameRect and just
replace
> > all
> > > the locations that touch it to m_ngPhysicalBoxFragment....
> > 
> > What should I do about layout calling location() (and I'm sure others)
before
> > LayoutBox has a m_ngPhysicalBoxFragment? Start with a placeholder and then
> have
> > NGFragmentBuilder replace it?
> 
> Oh sorry, i was imaginging for this initial test that a LayoutBox _always_ has
a
> PhysicalBoxFragment. so location/setLocation will always succeed. Does that
make
> sense?

I think so, but want to take a look and confirm?

> This would be without any layoutng code running.

[ikilpatrick, 2017-02-17 05:35:40]

yup, magic! now to work out a good benchmark....

[dgrogan, 2017-02-17 17:42:57]

On 2017/02/17 05:35:40, ikilpatrick wrote:
> yup, magic! now to work out a good benchmark....

I was planning on sifting through some of the smoothness telemetry benchmarks.
(I haven't used telemetry in 3 years though.) Unless you have a better avenue in
mind?

I have a nexus 4 I was planning on using as my low end device, but I don't know
about the memory bandwidth compared to moto e. But first, my desktop.