Prepare to use gfx::RenderText in views::Label.

Prepare to use gfx::RenderText in views::Label.

Alter Label's API to support a RenderText implementation.
This has minimal functional changes to Label for now.
The Label rewrite to use RenderText will land separately.

Remove the generally unused Label::SetDirectionalityMode.
We should always use the text's directionality anyway.
(it seems like this was the case even for *FROM_UI...)

Also remove views::LabelButton::SetDirectionalityMode.
Fix LabelButton mutli-line preferred sizing and layout.

Add RenderText flag to swap newline chars for symbols.
(We don't want that for multi-line views::Label layout)
Add GetCurrentHorizontalAlignment ALIGN_HEAD helper.
Expose ElideBehavior and ShadowValues accessors.
Set the styles of the argument FontList in SetFontList.
(this is how Label transmits style information for now)
Fix partial-pixel treatment for centered alignment.

Update Message Center's InnerBoundedLabel Label subclass.
(minimal changes to keep it working with older paint path)

BUG=240037
TEST=Negligible appearance changes (better multi-line label centering).
R=sky@chromium.org,asvitkine@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284401

[msw, 2014-07-14 18:31:31]

Hey Scott, please take a look; thanks!

[sky, 2014-07-14 21:49:57]

I wasn't entirely sure about some of the changes to app_list. Can you also add a
more local reviewer for those changes?

https://codereview.chromium.org/23228004/diff/466001/ui/views/controls/label.cc
File ui/views/controls/label.cc (left):

https://codereview.chromium.org/23228004/diff/466001/ui/views/controls/label....
ui/views/controls/label.cc:231: if ((!visible() && collapse_when_hidden_) ||
text_size.IsEmpty())
Does empty text still get a pref of 0x0?

https://codereview.chromium.org/23228004/diff/466001/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/23228004/diff/466001/ui/views/controls/label....
ui/views/controls/label.cc:293: Layout();
Seems better to me to delay layout. Is there a reason you need to layout
immediately.

https://codereview.chromium.org/23228004/diff/466001/ui/views/controls/label....
ui/views/controls/label.cc:303: PaintText(canvas);
Do any subclasses completely overriding PaintText so that they might now get a
double focus border?

https://codereview.chromium.org/23228004/diff/466001/ui/views/controls/label....
ui/views/controls/label.cc:333: InvalidateLayout();
I think you should InvalidateLayout() first. That way if an ancestor triggers a
layout during PreferredSizeChanged() this is layed out correctly. Similarly I
would only Layout() if still invalid.

[msw, 2014-07-14 23:41:36]

Comments addressed, please take another look.
+Trent for ui/app_list/views/app_list_item_view.*

Label color changes now schedules paint (see RecalculateColors), that made
AppListItemView::SchedulePaintInRect's color modifications trigger an infinite
loop. The Label usage there is pretty bad and could use some follow-up cleanup
like Patch Set 9, but I've kept the changes small here and preserved the
existing appearance.

https://codereview.chromium.org/23228004/diff/466001/ui/views/controls/label.cc
File ui/views/controls/label.cc (left):

https://codereview.chromium.org/23228004/diff/466001/ui/views/controls/label....
ui/views/controls/label.cc:231: if ((!visible() && collapse_when_hidden_) ||
text_size.IsEmpty())
On 2014/07/14 21:49:56, sky wrote:
> Does empty text still get a pref of 0x0?

Label used to return (0 x <font height>) for empty text. I restored that
behavior (needed for BoundedLabel, perhaps other users, at least for now) and
added a unit test.

https://codereview.chromium.org/23228004/diff/466001/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/23228004/diff/466001/ui/views/controls/label....
ui/views/controls/label.cc:293: Layout();
On 2014/07/14 21:49:56, sky wrote:
> Seems better to me to delay layout. Is there a reason you need to layout
> immediately.

I changed this to call ResetLayout(), which no longer calls Layout().

https://codereview.chromium.org/23228004/diff/466001/ui/views/controls/label....
ui/views/controls/label.cc:303: PaintText(canvas);
On 2014/07/14 21:49:56, sky wrote:
> Do any subclasses completely overriding PaintText so that they might now get a
> double focus border?

Nope, there are no Label::PaintText overrides. This also shouldn't causes issues
with Label::OnPaint overrides, but I'll handle any fallout if I'm wrong.

https://codereview.chromium.org/23228004/diff/466001/ui/views/controls/label....
ui/views/controls/label.cc:333: InvalidateLayout();
On 2014/07/14 21:49:56, sky wrote:
> I think you should InvalidateLayout() first. That way if an ancestor triggers
a
> layout during PreferredSizeChanged() this is layed out correctly. Similarly I
> would only Layout() if still invalid.

Done. I reordered InvalidateLayout() and removed the Layout() call.
(Layout seemed necessary in earlier iterations, but not now; huzzah!)

[tapted, 2014-07-15 00:52:39]

Probably orhogonal to this CL, but do you think we should ditch
app_list::CachedLabel? it exists purely as a performance optimization, which
sounds like it might now be redundant.

https://codereview.chromium.org/23228004/diff/506001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_item_view.cc (right):

https://codereview.chromium.org/23228004/diff/506001/ui/app_list/views/app_li...
ui/app_list/views/app_list_item_view.cc:213:
!apps_grid_view_->IsAnimatingView(this);
the hook was in a SchedulePaint override because the animating->not animating
transition was really hard to catch. This is when you release an item during
drag & drop and it snaps back to the grid (and it's a different animation on
CrOS/Linux vs Windows due to interaction with the OS drag and drop).

I'll patch this in and try it out, since it's not clear this is still handled.

If the schedule paint override is now a deal-breaker the app list might need a
change to each of the gfx::AnimationDelegates in apps_grid_view.cc

[msw, 2014-07-15 01:29:32]

On 2014/07/15 00:52:39, tapted wrote:
> Probably orhogonal to this CL, but do you think we should ditch
> app_list::CachedLabel? it exists purely as a performance optimization, which
> sounds like it might now be redundant.

Yeah, CachedLabel (and likely the whole pre-rendering codepath) should be
removed. See Patch Set 9, where I attempted this. I'm happy to review followup
work there, and might have a moment to help a little if needed.

https://codereview.chromium.org/23228004/diff/506001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_item_view.cc (right):

https://codereview.chromium.org/23228004/diff/506001/ui/app_list/views/app_li...
ui/app_list/views/app_list_item_view.cc:213:
!apps_grid_view_->IsAnimatingView(this);
On 2014/07/15 00:52:38, tapted wrote:
> the hook was in a SchedulePaint override because the animating->not animating
> transition was really hard to catch. This is when you release an item during
> drag & drop and it snaps back to the grid (and it's a different animation on
> CrOS/Linux vs Windows due to interaction with the OS drag and drop).
> 
> I'll patch this in and try it out, since it's not clear this is still handled.
> 
> If the schedule paint override is now a deal-breaker the app list might need a
> change to each of the gfx::AnimationDelegates in apps_grid_view.cc

Removing the SchedulePaint from Label::RecalculateColors seems wrong. If
AppListItemView really needs to change Label colors when paints are scheduled, I
could do a nontrivial amount of work to only SchedulePaint in
Label::RecalculateColors when the Label color values change (need to add
RenderText accessors and some label temporary values to check the
foreground/background colors and the subpixel_rendering_enabled_ flag). That
would probably alleviate the infinite loop. Changing the AnimationDelegates to
update the title label colors when the animation ends seems like a better
solution. Let me know what you think.

[msw, 2014-07-15 02:11:17]

https://codereview.chromium.org/23228004/diff/466001/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/23228004/diff/466001/ui/views/controls/label....
ui/views/controls/label.cc:293: Layout();
On 2014/07/14 23:41:35, msw wrote:
> On 2014/07/14 21:49:56, sky wrote:
> > Seems better to me to delay layout. Is there a reason you need to layout
> > immediately.
> 
> I changed this to call ResetLayout(), which no longer calls Layout().

Actually, I changed this to InvalidateLayout as you suggested. We shouldn't call
PreferredSizeChanged when the bounds change (unless perhaps the Label is
multi-line and the change of width causes a change of preferred height), but
LabelExample's multi-line label handles width changes correctly as-is.

[tapted, 2014-07-15 03:49:41]

On 2014/07/15 01:29:32, msw wrote:
> On 2014/07/15 00:52:39, tapted wrote:
> > Probably orhogonal to this CL, but do you think we should ditch
> > app_list::CachedLabel? it exists purely as a performance optimization, which
> > sounds like it might now be redundant.
> 
> Yeah, CachedLabel (and likely the whole pre-rendering codepath) should be
> removed. See Patch Set 9, where I attempted this. I'm happy to review followup
> work there, and might have a moment to help a little if needed.
> 
>
https://codereview.chromium.org/23228004/diff/506001/ui/app_list/views/app_li...
> File ui/app_list/views/app_list_item_view.cc (right):
> 
>
https://codereview.chromium.org/23228004/diff/506001/ui/app_list/views/app_li...
> ui/app_list/views/app_list_item_view.cc:213:
> !apps_grid_view_->IsAnimatingView(this);
> On 2014/07/15 00:52:38, tapted wrote:
> > the hook was in a SchedulePaint override because the animating->not
animating
> > transition was really hard to catch. This is when you release an item during
> > drag & drop and it snaps back to the grid (and it's a different animation on
> > CrOS/Linux vs Windows due to interaction with the OS drag and drop).
> > 
> > I'll patch this in and try it out, since it's not clear this is still
handled.
> > 
> > If the schedule paint override is now a deal-breaker the app list might need
a
> > change to each of the gfx::AnimationDelegates in apps_grid_view.cc
> 
> Removing the SchedulePaint from Label::RecalculateColors seems wrong. If
> AppListItemView really needs to change Label colors when paints are scheduled,
I
> could do a nontrivial amount of work to only SchedulePaint in
> Label::RecalculateColors when the Label color values change

What was lost when early-exiting from AppList's schedule-paint? That is,
(rightly or wrongly..) the app list currently disables the auto-color for
readability stuff for its labels -- the effective colours shouldn't need to
change. So would keeping the early-exit in AppList's schedule paint be able to
avoid the RecalculateColors trigger that's causing the loop?

Unfortunately there is a regression in the CL currently for the snap-back
animation after drag and drop. The text isn't nice on Windows without
subpixel-AA (otherwise it could probably be ignored). See e.g. Items 1/9 on
http://imgur.com/63M9Qzv 

Adding a call to SetTitleSubpixelAA() in apps_grid_view.cc's
ItemMoveAnimationDelegate could probably fix the regression, but you'd need to
expose it and do some static_cast-ing. The other AnimationDelegates don't seem
to need a fix.. but the scattered calls to SetTitleSubpixelAA feels more fragile
than hooking into schedulepaint.

[sky, 2014-07-15 04:36:45]

LGTM

[msw, 2014-07-15 05:00:52]

On 2014/07/15 03:49:41, tapted wrote:
> Unfortunately there is a regression in the CL currently for the snap-back
> animation after drag and drop. The text isn't nice on Windows without
> subpixel-AA (otherwise it could probably be ignored). See e.g. Items 1/9 on
> http://imgur.com/63M9Qzv 
> 
> Adding a call to SetTitleSubpixelAA() in apps_grid_view.cc's
> ItemMoveAnimationDelegate could probably fix the regression, but you'd need to
> expose it and do some static_cast-ing. The other AnimationDelegates don't seem
> to need a fix.. but the scattered calls to SetTitleSubpixelAA feels more
fragile
> than hooking into schedulepaint.

I think that's an acceptable temporary regression, especially given the already
poor state of the app list's Label code. Something like Patch Set 9 would be a
huge improvement there; I think we only need to disable subpixel AA during a
drag from within a folder (to show titles over the big gray circle). May I land
this Label change and address the app list afterward?

[msw, 2014-07-15 05:08:16]

Ah, and to address your more specific concerns:

On 2014/07/15 03:49:41, tapted wrote:
> What was lost when early-exiting from AppList's schedule-paint?

Items in folders looked to use subpixel AA on transparent backgrounds.
(perhaps this was the folder-reorg on open + caching images + ???)

> (rightly or wrongly..) the app list currently disables the auto-color for
> readability stuff for its labels -- the effective colours shouldn't need to
> change. So would keeping the early-exit in AppList's schedule paint be able to
> avoid the RecalculateColors trigger that's causing the loop?

No, Label needs to be repainted when its subpixel AA setting changes.

> Adding a call to SetTitleSubpixelAA() in apps_grid_view.cc's
> ItemMoveAnimationDelegate could probably fix the regression, but you'd need to
> expose it and do some static_cast-ing. The other AnimationDelegates don't seem
> to need a fix.. but the scattered calls to SetTitleSubpixelAA feels more
fragile
> than hooking into schedulepaint.

I'd rather rewrite the app list Label code afterward than add workarounds now.

[tapted, 2014-07-15 05:37:58]

app_list lgtm - I agree separate treatment of the label stuff in the app list is
a good idea, and this seems to behave for most cases.

On 2014/07/15 05:00:52, msw wrote:
> May I land this Label change and address the app list afterward?

sgtm

On 2014/07/15 05:08:16, msw wrote:
> Ah, and to address your more specific concerns:
> 
> On 2014/07/15 03:49:41, tapted wrote:
> > What was lost when early-exiting from AppList's schedule-paint?
> 
> Items in folders looked to use subpixel AA on transparent backgrounds.
> (perhaps this was the folder-reorg on open + caching images + ???)

Currently there is no subpixel AA within a folder - labels there are not given
an opaque background because dragging within a folder animates a growing bubble
that expands around (nearly) all the items -- some have a border across the text
:/. I haven't figured out an elegant way (i.e. only ugly ways..) to get subpixel
AA with this happening in folders, so we put up with gross fonts there on
Windows. This 'in a folder' state should be constant for the lifetime of the
AppListItemView, in case that helps. The lack-of-opaque-background is (was?)
currently enough to prevent attempts to render subpixel-AA on a
transparent/black background, without calling SetSubpixelRenderingEnabled
separately.

[msw, 2014-07-15 05:45:24]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-07-15 05:46:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/23228004/546001

[msw, 2014-07-15 08:23:45]

The CQ bit was unchecked by msw@chromium.org

[msw, 2014-07-15 08:26:15]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-07-15 08:26:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/23228004/566001

[commit-bot: I haz the power, 2014-07-15 14:29:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[Alexei Svitkine (slow), 2014-07-15 14:42:50]

LGTM % comments

Have you measured memory use after this? I'm worried that by keeping RenderText
instances around for each label, it might result in a non-trivial increase in
memory use since RenderText keeps a lot of internal state around (including
handles to opaque Uniscribe data).

One thing that I was thinking that can be done to improve that (in a follow-up
CL, not blocking this one), is that we can have a setter on RenderText that
specifies it's not editable and then RenderText can simply not keep around all
the state that's needed for cursoring, carets, etc.

I'm ok not blocking this CL for the memory use investigation, but I think it's
something we should measure in a follow-up. You could measure it by saving off a
profile with a session saved with a lot of tabs / bookmarks on the BMB. Then
restoring that profile from a version of Chrome that has that change and one
that doesn't and comparing memory use.

https://codereview.chromium.org/23228004/diff/566001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/23228004/diff/566001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:244: void SetReplaceNewlineCharsWithSymbols(bool replace);
Add a comment.

https://codereview.chromium.org/23228004/diff/566001/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/23228004/diff/566001/ui/views/controls/label....
ui/views/controls/label.cc:171: size.Enlarge(GetInsets().width(),
GetInsets().height());
Nit: Use a local var for insets like before to avoid recomputing them twice.

[commit-bot: I haz the power, 2014-07-15 15:07:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-15 15:07:34]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[msw, 2014-07-15 16:53:05]

Comments addressed.

On 2014/07/15 14:42:50, Alexei Svitkine wrote:
> Have you measured memory use after this? I'm worried that by keeping
RenderText
> instances around for each label, it might result in a non-trivial increase in
> memory use since RenderText keeps a lot of internal state around (including
> handles to opaque Uniscribe data).
> 
> One thing that I was thinking that can be done to improve that (in a follow-up
> CL, not blocking this one), is that we can have a setter on RenderText that
> specifies it's not editable and then RenderText can simply not keep around all
> the state that's needed for cursoring, carets, etc.
> 
> I'm ok not blocking this CL for the memory use investigation, but I think it's
> something we should measure in a follow-up. You could measure it by saving off
a
> profile with a session saved with a lot of tabs / bookmarks on the BMB. Then
> restoring that profile from a version of Chrome that has that change and one
> that doesn't and comparing memory use.

I'll look into memory usage later today and let you know.

https://codereview.chromium.org/23228004/diff/566001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/23228004/diff/566001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:244: void SetReplaceNewlineCharsWithSymbols(bool replace);
On 2014/07/15 14:42:50, Alexei Svitkine wrote:
> Add a comment.

Done.

https://codereview.chromium.org/23228004/diff/566001/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/23228004/diff/566001/ui/views/controls/label....
ui/views/controls/label.cc:171: size.Enlarge(GetInsets().width(),
GetInsets().height());
On 2014/07/15 14:42:50, Alexei Svitkine wrote:
> Nit: Use a local var for insets like before to avoid recomputing them twice.

Done.

[msw, 2014-07-15 16:53:14]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-07-15 16:55:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/23228004/586001

[msw, 2014-07-15 18:46:35]

On 2014/07/15 16:53:05, msw wrote:
> Comments addressed.
> 
> On 2014/07/15 14:42:50, Alexei Svitkine wrote:
> > Have you measured memory use after this? I'm worried that by keeping
> RenderText
> > instances around for each label, it might result in a non-trivial increase
in
> > memory use since RenderText keeps a lot of internal state around (including
> > handles to opaque Uniscribe data).
> > 
> > One thing that I was thinking that can be done to improve that (in a
follow-up
> > CL, not blocking this one), is that we can have a setter on RenderText that
> > specifies it's not editable and then RenderText can simply not keep around
all
> > the state that's needed for cursoring, carets, etc.
> > 
> > I'm ok not blocking this CL for the memory use investigation, but I think
it's
> > something we should measure in a follow-up. You could measure it by saving
off
> a
> > profile with a session saved with a lot of tabs / bookmarks on the BMB. Then
> > restoring that profile from a version of Chrome that has that change and one
> > that doesn't and comparing memory use.
> 
> I'll look into memory usage later today and let you know.

I ran two informal tests three times each with and without my patch:

A) Run chrome fullscreen with the bookmark bar full of about:blank and
about:crash (17 items + Apps + Other Bookmarks), open 10 tabs of each from the
"Open All Bookmarks" context menu on a subfolder of Other Bookmarks, open task
manager, wait for tabs to load and memory usage to settle reasonably. Record the
Browser Memory usage as reported by the Task Manager.
Results before: 429176K, 430100K, 428876K (429384K avg)
Results after:  429184K, 433156K, 432508K (431616K avg)

B) Run views_examples_exe.exe, open the Label example, drag the window (whereby
it resizes to fit the content). Record Windows Task Manager process memory
(private working set).
Results before: 94216K, 93588K, 93676K (93826.66K avg)
Results after:  94256K, 93936K, 92928K (93706.66K avg)

Obviously, this is a very limited sample of a flawed and limited test, so the
results aren't conclusive and shouldn't be too meaningful. The results between
chrome and views_examples_exe even offer conflicting deltas between the average
values. Note that switching to RenderTextHarfBuzz and eventually supporting
multiline RenderText instances (instead of creating an array of instances in
Label) may each have a greater effect than this change. Your point about better
managing RenderText internal cached data may also yeild some measurable change.
I think the only point worth noting here is that there doesn't seem to be any
significant obvious increase in memory usage that ought to block this
performance and cleanup CL.

[Alexei Svitkine (slow), 2014-07-15 18:58:37]

Can you also check with i18n text? Also, I think using Chrome is more
meaningful as a test than views examples, since Chrome has much more
instances of labels than views_examples I think.

I'd really like to see more data from Chrome since a 2MB difference is not
something I would consider trivial. So I'd like to see more runs to see if
it's really noise or if it's a real effect.


On Tue, Jul 15, 2014 at 2:46 PM, <msw@chromium.org> wrote:

> On 2014/07/15 16:53:05, msw wrote:
>
>> Comments addressed.
>>
>
>  On 2014/07/15 14:42:50, Alexei Svitkine wrote:
>> > Have you measured memory use after this? I'm worried that by keeping
>> RenderText
>> > instances around for each label, it might result in a non-trivial
>> increase
>>
> in
>
>> > memory use since RenderText keeps a lot of internal state around
>> (including
>> > handles to opaque Uniscribe data).
>> >
>> > One thing that I was thinking that can be done to improve that (in a
>>
> follow-up
>
>> > CL, not blocking this one), is that we can have a setter on RenderText
>> that
>> > specifies it's not editable and then RenderText can simply not keep
>> around
>>
> all
>
>> > the state that's needed for cursoring, carets, etc.
>> >
>> > I'm ok not blocking this CL for the memory use investigation, but I
>> think
>>
> it's
>
>> > something we should measure in a follow-up. You could measure it by
>> saving
>>
> off
>
>> a
>> > profile with a session saved with a lot of tabs / bookmarks on the BMB.
>> Then
>> > restoring that profile from a version of Chrome that has that change
>> and one
>> > that doesn't and comparing memory use.
>>
>
>  I'll look into memory usage later today and let you know.
>>
>
> I ran two informal tests three times each with and without my patch:
>
> A) Run chrome fullscreen with the bookmark bar full of about:blank and
> about:crash (17 items + Apps + Other Bookmarks), open 10 tabs of each from
> the
> "Open All Bookmarks" context menu on a subfolder of Other Bookmarks, open
> task
> manager, wait for tabs to load and memory usage to settle reasonably.
> Record the
> Browser Memory usage as reported by the Task Manager.
> Results before: 429176K, 430100K, 428876K (429384K avg)
> Results after:  429184K, 433156K, 432508K (431616K avg)
>
> B) Run views_examples_exe.exe, open the Label example, drag the window
> (whereby
> it resizes to fit the content). Record Windows Task Manager process memory
> (private working set).
> Results before: 94216K, 93588K, 93676K (93826.66K avg)
> Results after:  94256K, 93936K, 92928K (93706.66K avg)
>
> Obviously, this is a very limited sample of a flawed and limited test, so
> the
> results aren't conclusive and shouldn't be too meaningful. The results
> between
> chrome and views_examples_exe even offer conflicting deltas between the
> average
> values. Note that switching to RenderTextHarfBuzz and eventually supporting
> multiline RenderText instances (instead of creating an array of instances
> in
> Label) may each have a greater effect than this change. Your point about
> better
> managing RenderText internal cached data may also yeild some measurable
> change.
> I think the only point worth noting here is that there doesn't seem to be
> any
> significant obvious increase in memory usage that ought to block this
> performance and cleanup CL.
>
> https://codereview.chromium.org/23228004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[msw, 2014-07-15 22:35:23]

On 2014/07/15 18:58:37, Alexei Svitkine wrote:
> Can you also check with i18n text? Also, I think using Chrome is more
> meaningful as a test than views examples, since Chrome has much more
> instances of labels than views_examples I think.
> 
> I'd really like to see more data from Chrome since a 2MB difference is not
> something I would consider trivial. So I'd like to see more runs to see if
> it's really noise or if it's a real effect.

On 2014/07/15 18:58:37, Alexei Svitkine wrote:
> Can you also check with i18n text? Also, I think using Chrome is more
> meaningful as a test than views examples, since Chrome has much more
> instances of labels than views_examples I think.
> 
> I'd really like to see more data from Chrome since a 2MB difference is not
> something I would consider trivial. So I'd like to see more runs to see if
> it's really noise or if it's a real effect.

I ran a new naive test for many i18n labels, you can run more if you want. I
revised LabelExample to use *350* Labels, containing *many* languages, from the
http://www.wikipedia.org/ homepage (the titles of Wikipedia's available
languages in their native character sets, etc.). I took 6 measurements in 5
repeated tests with and without my patch. I ran views_examples_exe, opened Label
Example (1), scrolled to the bottom (2), scrolled back to the top (3), resized
the window to be tall and thin (4), maximized and restored the window (5), and
resized the window like mad (6). (during resize drag loops, I saw numbers in the
high 120,000 range).

Here are the results without my Label patch:
  (1),   (2),   (3),    (4),    (5),    (6)
94388, 94680, 94712,  98800, 109856, 115444
94380, 94412, 94952, 108328, 110948, 117656
94304, 94644, 94936, 102428, 109412, 120416
94104, 94512, 94632, 105464, 109304, 119728
94076, 94620, 94656, 103520, 108896, 122212

Here are the results *with* my Label patch:
  (1),   (2),   (3),    (4),    (5),    (6)
95624, 96484, 95968, 103844, 111724, 112472
95504, 96400, 97292, 107408, 110576, 121844
95792, 95936, 96188, 105876, 112632, 123328
95916, 96132, 97004, 106400, 111180, 120896
95848, 96824, 96212, 102372, 114732, 119412

The numbers do show an increased memory usage, which is consistent with the
persistence of RenderText instances throughout the Label lifetime. Sadly, this
CL alone doesn't seem to resolve the performance concerns of Issue 240037
(removing the tab titles altogether makes tab dragging much more responsive).
Regardless, I think this CL is a move in the right direction for performance and
code health, but I'll abandon this line of work if you all disagree.

[Alexei Svitkine (slow), 2014-07-15 23:08:08]

I don't think this work should be abandoned. I think it's in the right
direction and we should proceed with it.

From the above test, it looks like there's a 2MB average difference between
the two (assuming units are KB). I think we can reduce this further by
pursuing the size optimizations I mentioned above (i.e. a "non-editable"
mode which throws away a lot of unnecessary intermediate state). Then, we
can see how it affects the above measurements. I'm okay with this being
done in a follow-up CL as long as it makes it for M38.

(As for the perf issue, perhaps we should still pursue more aggressive
caching approaches like the one I had originally looked at - either caching
whole runs as I suggested before - or possibly a smarter font fallback
cache - possibly keyed by strings/chars.)


On Tue, Jul 15, 2014 at 6:35 PM, <msw@chromium.org> wrote:

> On 2014/07/15 18:58:37, Alexei Svitkine wrote:
>
>> Can you also check with i18n text? Also, I think using Chrome is more
>> meaningful as a test than views examples, since Chrome has much more
>> instances of labels than views_examples I think.
>>
>
>  I'd really like to see more data from Chrome since a 2MB difference is not
>> something I would consider trivial. So I'd like to see more runs to see if
>> it's really noise or if it's a real effect.
>>
>
> On 2014/07/15 18:58:37, Alexei Svitkine wrote:
>
>> Can you also check with i18n text? Also, I think using Chrome is more
>> meaningful as a test than views examples, since Chrome has much more
>> instances of labels than views_examples I think.
>>
>
>  I'd really like to see more data from Chrome since a 2MB difference is not
>> something I would consider trivial. So I'd like to see more runs to see if
>> it's really noise or if it's a real effect.
>>
>
> I ran a new naive test for many i18n labels, you can run more if you want.
> I
> revised LabelExample to use *350* Labels, containing *many* languages,
> from the
> http://www.wikipedia.org/ homepage (the titles of Wikipedia's available
> languages in their native character sets, etc.). I took 6 measurements in 5
> repeated tests with and without my patch. I ran views_examples_exe, opened
> Label
> Example (1), scrolled to the bottom (2), scrolled back to the top (3),
> resized
> the window to be tall and thin (4), maximized and restored the window (5),
> and
> resized the window like mad (6). (during resize drag loops, I saw numbers
> in the
> high 120,000 range).
>
> Here are the results without my Label patch:
>   (1),   (2),   (3),    (4),    (5),    (6)
> 94388, 94680, 94712,  98800, 109856, 115444
> 94380, 94412, 94952, 108328, 110948, 117656
> 94304, 94644, 94936, 102428, 109412, 120416
> 94104, 94512, 94632, 105464, 109304, 119728
> 94076, 94620, 94656, 103520, 108896, 122212
>
> Here are the results *with* my Label patch:
>   (1),   (2),   (3),    (4),    (5),    (6)
> 95624, 96484, 95968, 103844, 111724, 112472
> 95504, 96400, 97292, 107408, 110576, 121844
> 95792, 95936, 96188, 105876, 112632, 123328
> 95916, 96132, 97004, 106400, 111180, 120896
> 95848, 96824, 96212, 102372, 114732, 119412
>
> The numbers do show an increased memory usage, which is consistent with the
> persistence of RenderText instances throughout the Label lifetime. Sadly,
> this
> CL alone doesn't seem to resolve the performance concerns of Issue 240037
> (removing the tab titles altogether makes tab dragging much more
> responsive).
> Regardless, I think this CL is a move in the right direction for
> performance and
> code health, but I'll abandon this line of work if you all disagree.
>
> https://codereview.chromium.org/23228004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-07-16 05:29:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[commit-bot: I haz the power, 2014-07-16 06:09:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-16 06:09:48]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[msw, 2014-07-17 17:36:03]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-07-17 17:37:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/23228004/586001

[commit-bot: I haz the power, 2014-07-17 20:26:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[commit-bot: I haz the power, 2014-07-17 21:06:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-17 21:07:01]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[msw, 2014-07-21 05:44:38]

I reverted the Label implementation changes and the app list changes, this is
now a preparatory CL for an impending Label rewrite. I've left the Label
interface changes, supporting RenderText changes, and some minor improvements
and cleanup.

This is primarily in response to the numerous browser_tests and
interactive_ui_tests timeouts on win_chromium_rel seen in earlier patch sets.
I'll attempt to address our memory and performance concerns before landing a
Label rewrite that caches RenderText objects.

I'm sending it to the CQ, but feel free to take another look at this simplified
CL, thanks for bearing with me here.

[msw, 2014-07-21 05:44:51]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-07-21 05:45:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/23228004/666001

[commit-bot: I haz the power, 2014-07-21 07:01:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

[commit-bot: I haz the power, 2014-07-21 07:55:56]

Change committed as 284401