Avoid scrollbar construction/destruction thrashing during flex layout.

Avoid scrollbar construction/destruction thrashing during flex layout.

BUG=528940
R=skobes@chromium.org,pdr@chromium.org

Before this change:

https://codereview.chromium.org/1295933003

... blocks with overflow:auto would delay updating their scrollbars until
after all flex layout was finished.  After the change, scrollbar info is
updated immediately during the course of layout.  Flex items may run
layout multiple times during flex layout; if a flex item has auto scrollbars,
it may create and destroy its scrollbars multiple times.

Aside from being a performance problem, this can cause WebScrollbarThemePainter
to point to a stale Scrollbar object.  If a flex item has scrollbars prior to
layout; then the flex item destroys and creates scrollbars during layout; and
at the end of layout, it still has a scrollbar; then
CompositedDeprecatedPaintLayerMapping::updateOverflowControlsLayers will not
update the WebScrollbarThemePainter with the final Scrollbar object.

We could fix this in updateOverflowControlsLayers, but that wouldn't address
the performance issue of needlessly creating and destroying scrollbars during
flex layout.  This patch avoids destroying scrollbars that are no longer
deemed necessary, until after all flexing is finished.

[skobes, 2015-09-17 00:01:16]

I'm not a big fan of this change.  It's confusing to have two different ways in
which a scrollbar can be present / absent, and it seems brittle if the
DeprecatedPaintLayerScrollableArea has to be careful not to directly access
m_hBar and m_vBar.

There must be a cleaner way to avoid duplicate Scrollbar creation.  What if we
had m_needsHBar and m_needsVBar which act like dirty bits, and defer the actual
scrollbar creation until after all the layouts are done?

[szager1, 2015-09-17 00:20:46]

On 2015/09/17 00:01:16, skobes wrote:
> I'm not a big fan of this change.  It's confusing to have two different ways
in
> which a scrollbar can be present / absent, and it seems brittle if the
> DeprecatedPaintLayerScrollableArea has to be careful not to directly access
> m_hBar and m_vBar.
> 
> There must be a cleaner way to avoid duplicate Scrollbar creation.  What if we
> had m_needsHBar and m_needsVBar which act like dirty bits, and defer the
actual
> scrollbar creation until after all the layouts are done?

That doesn't work, because we need to have constructed scrollbars so we can add
their width to the logical height of the block for layout.

It also doesn't work because scrollbars would still be destroyed during the
first phase of flexing (when flex items are laid out at full preferred
width/height).  So, there would still be the problem that scrollbar instance at
the end of layout would be different from the scrollbar instance that existed
before layout.  We would have to do something in
CompositedDeprecatedPaintLayerMapping::updateOverflowControlsLayers to associate
the new scrollbar instance with the WebScrollbarThemePainter.

And even if we figured all that stuff out, we would *still* be needlessly
destroying and recreating a scrollbar.

[szager1, 2015-09-17 00:21:54]

> 
> That doesn't work, because we need to have constructed scrollbars so we can
add
> their width to the logical height of the block for layout.

I meant: "add their width to the logical >width< of the block for layout."

[szager1, 2015-09-17 00:26:52]

On 2015/09/17 00:01:16, skobes wrote:
> I'm not a big fan of this change.  It's confusing to have two different ways
in
> which a scrollbar can be present / absent, and it seems brittle if the
> DeprecatedPaintLayerScrollableArea has to be careful not to directly access
> m_hBar and m_vBar.

m_hBar and m_vBar are only unreliable while flex layout is happening.  Maybe
there's some way we can limit the interface that DPLScrollableArea provides to
the layout code?

Another solution would be to stash two more Scrollbar pointers somewhere, and
set m_hBar/m_vBar to nullptr.  That would mean that every time createScrollbar
is called, it would have to check to see if there are existing scrollbars
stashed away and use them.  That would address the issue of m_hBar and m_vBar
being volatile, but it would be a bit wasteful of space.

[skobes, 2015-09-17 00:29:52]

On 2015/09/17 00:20:46, szager1 wrote:
> On 2015/09/17 00:01:16, skobes wrote:
> > I'm not a big fan of this change.  It's confusing to have two different ways
> in
> > which a scrollbar can be present / absent, and it seems brittle if the
> > DeprecatedPaintLayerScrollableArea has to be careful not to directly access
> > m_hBar and m_vBar.
> > 
> > There must be a cleaner way to avoid duplicate Scrollbar creation.  What if
we
> > had m_needsHBar and m_needsVBar which act like dirty bits, and defer the
> actual
> > scrollbar creation until after all the layouts are done?
> 
> That doesn't work, because we need to have constructed scrollbars so we can
add
> their width to the logical height of the block for layout.
> 
> It also doesn't work because scrollbars would still be destroyed during the
> first phase of flexing (when flex items are laid out at full preferred
> width/height).  So, there would still be the problem that scrollbar instance
at
> the end of layout would be different from the scrollbar instance that existed
> before layout.  We would have to do something in
> CompositedDeprecatedPaintLayerMapping::updateOverflowControlsLayers to
associate
> the new scrollbar instance with the WebScrollbarThemePainter.
> 
> And even if we figured all that stuff out, we would *still* be needlessly
> destroying and recreating a scrollbar.

Well, my proposal was more along the lines of moving scrollbar creation /
destruction out into a distinct phase separate from layout.  But it does assume
we can determine the width / height of a scrollbar without having to create it,
which is perhaps non-trivial.

Is this really only for flexboxes?  Can we isolate the complexity to the flexbox
code somehow?

[skobes, 2015-09-17 00:33:59]

On 2015/09/17 00:29:52, skobes wrote:
> On 2015/09/17 00:20:46, szager1 wrote:
> > On 2015/09/17 00:01:16, skobes wrote:
> > > I'm not a big fan of this change.  It's confusing to have two different
ways
> > in
> > > which a scrollbar can be present / absent, and it seems brittle if the
> > > DeprecatedPaintLayerScrollableArea has to be careful not to directly
access
> > > m_hBar and m_vBar.
> > > 
> > > There must be a cleaner way to avoid duplicate Scrollbar creation.  What
if
> we
> > > had m_needsHBar and m_needsVBar which act like dirty bits, and defer the
> > actual
> > > scrollbar creation until after all the layouts are done?
> > 
> > That doesn't work, because we need to have constructed scrollbars so we can
> add
> > their width to the logical height of the block for layout.
> > 
> > It also doesn't work because scrollbars would still be destroyed during the
> > first phase of flexing (when flex items are laid out at full preferred
> > width/height).  So, there would still be the problem that scrollbar instance
> at
> > the end of layout would be different from the scrollbar instance that
existed
> > before layout.  We would have to do something in
> > CompositedDeprecatedPaintLayerMapping::updateOverflowControlsLayers to
> associate
> > the new scrollbar instance with the WebScrollbarThemePainter.
> > 
> > And even if we figured all that stuff out, we would *still* be needlessly
> > destroying and recreating a scrollbar.
> 
> Well, my proposal was more along the lines of moving scrollbar creation /
> destruction out into a distinct phase separate from layout.  But it does
assume
> we can determine the width / height of a scrollbar without having to create
it,
> which is perhaps non-trivial.
> 
> Is this really only for flexboxes?  Can we isolate the complexity to the
flexbox
> code somehow?

FrameView has a multi pass update process for scrollbar existence:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I wonder how it avoids this issue?  As far as I can tell it will recreate the
Scrollbar whenever it is toggled by setHasHorizontalScrollbar /
setHasVerticalScrollbar.

[szager1, 2015-09-21 21:21:16]

On 2015/09/17 00:33:59, skobes wrote:

> FrameView has a multi pass update process for scrollbar existence:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> I wonder how it avoids this issue?  As far as I can tell it will recreate the
> Scrollbar whenever it is toggled by setHasHorizontalScrollbar /
> setHasVerticalScrollbar.

I'm working on a new version of the patch, but just to answer this question: as
far as I can tell, the FrameView does not use the same code to associate a
Scrollbar with a WebScrollbarThemePainter as DPLSA does.  I think the FrameView
might indeed do some unnecessary destruction/creation of scrollbars, but the
amount of extra work is small and bounded (unlike DPLSA).

[szager1, 2015-09-23 00:52:49]

FYI, here is a version of the patch that passes tests.  I'm now working on a
version that abstracts out the Scrollbar lifetime management into a separate
class, to make it more explicit what's going on.

[szager1, 2015-09-23 05:55:37]

The latest patch adds a ScrollbarManager class nested inside DPLSA, which
handles the "suppress" functionality.  I'm not sure I like this; WDYT?

[skobes, 2015-09-23 16:01:30]

On 2015/09/23 05:55:37, szager1 wrote:
> The latest patch adds a ScrollbarManager class nested inside DPLSA, which
> handles the "suppress" functionality.  I'm not sure I like this; WDYT?

Do you know the performance impact of the unnecessary blink::Scrollbar
construction?  This feels a bit like a premature optimization, if a simpler
solution exists by updating WebScrollbarThemePainter with the new instance.

Assuming we want to incur additional complexity to avoid the extra construction,
I like the idea of isolating it to a helper class, but I still think the
"suppress" concept is confusing, and would prefer a positive boolean like
"attached" (createScrollbar / attachScrollbar / detachScrollbar /
destroyScrollbar).

[szager1, 2015-09-23 16:49:22]

On 2015/09/23 16:01:30, skobes wrote:
> On 2015/09/23 05:55:37, szager1 wrote:
> > The latest patch adds a ScrollbarManager class nested inside DPLSA, which
> > handles the "suppress" functionality.  I'm not sure I like this; WDYT?
> 
> Do you know the performance impact of the unnecessary blink::Scrollbar
> construction?  This feels a bit like a premature optimization, if a simpler
> solution exists by updating WebScrollbarThemePainter with the new instance.

My previous flexbox change caused a performance regression of about 2% on just
the inspector tests; other tests were essentially unaffected.  This makes sense
because the inspector uses flexbox extensively.  That 2% regression was measured
in aggregate over multiple runs of all the inspector tests; the regression on
individual inspectors tests was highly variable, with the worst-affected tests
running more than twice as slowly.

Today seems like a good day to generate some more performance numbers, so I'll
go ahead and do that, and I'll work on a patch that s/suppress/attach/.

> 
> Assuming we want to incur additional complexity to avoid the extra
construction,
> I like the idea of isolating it to a helper class, but I still think the
> "suppress" concept is confusing, and would prefer a positive boolean like
> "attached" (createScrollbar / attachScrollbar / detachScrollbar /
> destroyScrollbar).

[szager1, 2015-09-23 22:00:08]

Post-blink-merge, migrating this to:

https://codereview.chromium.org/1357423007