[WIP: not for review] Reduce re-layout

[WIP: not for review] Reduce re-layout


Add multi-line elision support for RenderText

Omnibox "answers in suggest" needs to support the new
dictionary definition answer type, which requires
multi-line elision support from RenderText. This CL is
just the RenderText support changes.

The strategy employed is to render the text into an
unlimited number of lines (like the current unelided
method) then to elide the last line supported (if
necessary.)

BUG=591803
patch from issue 1868183002 at patchset 200001 (http://crrev.com/1868183002#ps200001)

[oshima, 2016-05-06 04:47:10]

Description was changed from

==========
Optimize


Add multi-line elision support for RenderText

Omnibox "answers in suggest" needs to support the new
dictionary definition answer type, which requires
multi-line elision support from RenderText. This CL is
just the RenderText support changes.

The strategy employed is to render the text into an
unlimited number of lines (like the current unelided
method) then to elide the last line supported (if
necessary.)

BUG=591803

patch from issue 1868183002 at patchset 200001
(http://crrev.com/1868183002#ps200001)
==========

to

==========
Reduce re-layout


Add multi-line elision support for RenderText

Omnibox "answers in suggest" needs to support the new
dictionary definition answer type, which requires
multi-line elision support from RenderText. This CL is
just the RenderText support changes.

The strategy employed is to render the text into an
unlimited number of lines (like the current unelided
method) then to elide the last line supported (if
necessary.)

BUG=591803

patch from issue 1868183002 at patchset 200001
(http://crrev.com/1868183002#ps200001)
==========

[oshima, 2016-05-06 04:47:11]

oshima@chromium.org changed reviewers:
+ tapted@chromium.org

[oshima, 2016-05-06 04:47:30]

Description was changed from

==========
Reduce re-layout


Add multi-line elision support for RenderText

Omnibox "answers in suggest" needs to support the new
dictionary definition answer type, which requires
multi-line elision support from RenderText. This CL is
just the RenderText support changes.

The strategy employed is to render the text into an
unlimited number of lines (like the current unelided
method) then to elide the last line supported (if
necessary.)

BUG=591803

patch from issue 1868183002 at patchset 200001
(http://crrev.com/1868183002#ps200001)
==========

to

==========
[WIP: not for review] Reduce re-layout


Add multi-line elision support for RenderText

Omnibox "answers in suggest" needs to support the new
dictionary definition answer type, which requires
multi-line elision support from RenderText. This CL is
just the RenderText support changes.

The strategy employed is to render the text into an
unlimited number of lines (like the current unelided
method) then to elide the last line supported (if
necessary.)

BUG=591803

patch from issue 1868183002 at patchset 200001
(http://crrev.com/1868183002#ps200001)
==========

[oshima, 2016-05-06 04:47:58]

tapted@, can you try this on mac and see if this is better?

thanks

[tapted, 2016-05-09 08:06:29]

On 2016/05/06 04:47:58, oshima wrote:
> tapted@, can you try this on mac and see if this is better?
> 
> thanks

Sorry - I'll try to get to this tomorrow, and do some analysis. But did you see
the trace I added to http://crbug.com/608990 , from the earlier patch? From the
initial glance, I got the impression from that trace that the bottlenecks were
for multiline stuff in general -- not necessarily the eliding, but I haven't had
time to dive in properly - my task queue has exploded over the last few workdays
:/

[oshima, 2016-05-09 08:42:18]

On 2016/05/09 08:06:29, tapted wrote:
> On 2016/05/06 04:47:58, oshima wrote:
> > tapted@, can you try this on mac and see if this is better?
> > 
> > thanks
> 
> Sorry - I'll try to get to this tomorrow, and do some analysis. But did you
see
> the trace I added to http://crbug.com/608990 , from the earlier patch? From
the
> initial glance, I got the impression from that trace that the bottlenecks were
> for multiline stuff in general -- not necessarily the eliding, 

Yes, and we may be triggering this slow multi-line layout more than necessary
when eliding.

> but I haven't had
> time to dive in properly - my task queue has exploded over the last few
workdays
> :/

This isn't high priority, so only if you have time.

[tapted, 2016-05-10 08:45:53]

Attached another trace to http://crbug.com/608990 . I'm not sure how much better
it is to be honest - it's still quite bad -- lagging behind by ~10 seconds after
putting in a second line of text. Comments below about what's eating up all the
CPU.

https://codereview.chromium.org/1953133002/diff/1/ui/views/examples/multiline...
File ui/views/examples/multiline_example.cc (right):

https://codereview.chromium.org/1953133002/diff/1/ui/views/examples/multiline...
ui/views/examples/multiline_example.cc:72:
render_text_->ApplyStyle(gfx::DIAGONAL_STRIKE, true, color_range);
One big factor seems to be these 4 "ApplyStyle" calls. There's a TODO in
render_text.cc

  // TODO(oshima|msw): Not all style change requires layout changes.
  // Consider optimizing based on the type of change.
  OnLayoutTextAttributeChanged(false);

Each time, there's a call to RTHB::IsValidCursorIndex which triggers
GetDisplayText and a full layout. This accounts for 56% of the CPU time, but it
could be a quarter of that if the styles were all applied "at once".

UpdateDisplayText shows up with 7 calls to RenderText::Elide (under
render_text.cc:1086 in UpdateDisplayText) accounting for about ~2/3 the time.
The rest is in the EnsureLayout call in UpdateDisplayText (line 1078)

Each of these goes to EnsureLayout goes to ShapeRun[WithFont] which goes to
hb_shape(..) that eats up CPU.

https://codereview.chromium.org/1953133002/diff/1/ui/views/examples/multiline...
ui/views/examples/multiline_example.cc:178: container()->Layout();
about 38% of the CPU time is under this call. This triggers
RTHB::EnsureLayoutRunList, which does UpdateDisplayText (twice? rthb.cc:1086 and
:1078) that accounts for ~60% of this 38%. The rest is under RTHB::EnsureLayout.

Only 29/886 of samples (3% of that 38%, or 1.26% total) is spent in the
Mac-specific gfx::GetFallbackFonts

The non-multiline views::Label::GetHeightForWidth call is 65/886 samples under
this. So over 10 times faster.

The repaint that occurs for the views::TextField (also not multi-line) is not
under this, but it's similar (88 samples, or about 4%.

So these two comments account for about 56+38+4=98% of the CPU time. The rest is
random smaller things, including painting

[tapted, 2016-05-11 00:16:26]

https://codereview.chromium.org/1953133002/diff/1/ui/gfx/render_text_harfbuzz.cc
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1953133002/diff/1/ui/gfx/render_text_harfbuzz...
ui/gfx/render_text_harfbuzz.cc:1572: GetDisplayText(),
So thinking about it some more - I think this is the biggest culprit.

This gets called from ApplyStyle via IsValidCursorIndex(..) (_before_ the style
is applied even).

There's a comment "Do not change styles mid-grapheme to avoid breaking
ligatures", so I guess we can't just switch to using IsValidLogicalIndex in
ApplyStyle(). But does it make sense to validate the range of the style being
applied based on the _elided_ text?

Should there be something like IsValidLayoutIndex(..) which validates the style
range based on the layout_text() instead of the display_text()? This is all
needed _before_ the style is applied so the layout_text() could immediately
change anyway, meaning a previously valid index is now elided once the style is
applied.

Of course, we'd also need an updated `IndexOfAdjacentGrapheme(..)` to support
this use-case as well.

[oshima, 2016-05-11 15:10:47]

On 2016/05/11 00:16:26, tapted wrote:
>
https://codereview.chromium.org/1953133002/diff/1/ui/gfx/render_text_harfbuzz.cc
> File ui/gfx/render_text_harfbuzz.cc (right):
> 
>
https://codereview.chromium.org/1953133002/diff/1/ui/gfx/render_text_harfbuzz...
> ui/gfx/render_text_harfbuzz.cc:1572: GetDisplayText(),
> So thinking about it some more - I think this is the biggest culprit.
> 
> This gets called from ApplyStyle via IsValidCursorIndex(..) (_before_ the
style
> is applied even).
> 
> There's a comment "Do not change styles mid-grapheme to avoid breaking
> ligatures", so I guess we can't just switch to using IsValidLogicalIndex in
> ApplyStyle(). But does it make sense to validate the range of the style being
> applied based on the _elided_ text?
> 
> Should there be something like IsValidLayoutIndex(..) which validates the
style
> range based on the layout_text() instead of the display_text()? This is all
> needed _before_ the style is applied so the layout_text() could immediately
> change anyway, meaning a previously valid index is now elided once the style
is
> applied.
> 
> Of course, we'd also need an updated `IndexOfAdjacentGrapheme(..)` to support
> this use-case as well.

I'll look into more detail next week. I probably should test it on mac.

[oshima, 2016-05-11 15:10:48]

On 2016/05/11 00:16:26, tapted wrote:
>
https://codereview.chromium.org/1953133002/diff/1/ui/gfx/render_text_harfbuzz.cc
> File ui/gfx/render_text_harfbuzz.cc (right):
> 
>
https://codereview.chromium.org/1953133002/diff/1/ui/gfx/render_text_harfbuzz...
> ui/gfx/render_text_harfbuzz.cc:1572: GetDisplayText(),
> So thinking about it some more - I think this is the biggest culprit.
> 
> This gets called from ApplyStyle via IsValidCursorIndex(..) (_before_ the
style
> is applied even).
> 
> There's a comment "Do not change styles mid-grapheme to avoid breaking
> ligatures", so I guess we can't just switch to using IsValidLogicalIndex in
> ApplyStyle(). But does it make sense to validate the range of the style being
> applied based on the _elided_ text?
> 
> Should there be something like IsValidLayoutIndex(..) which validates the
style
> range based on the layout_text() instead of the display_text()? This is all
> needed _before_ the style is applied so the layout_text() could immediately
> change anyway, meaning a previously valid index is now elided once the style
is
> applied.
> 
> Of course, we'd also need an updated `IndexOfAdjacentGrapheme(..)` to support
> this use-case as well.

I'll look into more detail next week. I probably should test it on mac.