1045858 - Homescreen with 150 apps takes 500ms restyling over two restyles

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Homescreen, defect, P3)

Description

[Benoit Girard (:BenWa)]

Profile: http://people.mozilla.org/~bgirard/cleopatra/#report=46f55482af4f58a113e3a4c81ffe24f69fc911f2

It feels like there's something out of control here. If you amortize the restyle time per app it's still 3ms per app restyle which seems much too high.

StyleText (GetShadowData) takes a big chunk.
StartBackgroundImageLoads is fairly big as well.

:dholbert could the app be doing something unresonable or should we look to optimize the platform here?

Comment 1

[Benoit Girard (:BenWa)]

And forgot to add. For STR see: https://bugzilla.mozilla.org/show_bug.cgi?id=1045852#c0

Comment 2

[Daniel Holbert [:dholbert]]

At a high level, it looks like the first style-flush (in Refresh 0) is mostly frame-construction. (ProcessPendingRestyles takes up 56.4% of the samples inside of "Styles #0", and its call to nsCSSFrameConstructor::CreateNeededFrames() is responsible for virtually all of that (56% of the samples)).

That seems like the sort of thing that's unlikely to be a pathological case for the platform, perf-wise, though I'm not sure.  bz knows the frame constructor code better than anyone; maybe he has some thoughts.

Per comment 0, a lot of time (about 1/3 of the profile's first restyle, I think) is spent in StyleText().  That time is *really* all spent in CalcLengthWith (down a few levels)  And particularly, in style-resolution code (WalkRuleTree, ResolveStyleFor) inside of CalcLengthWith.  This means that this time is being spent resolving lengths that have em/ex/ch units -- the chunk below here:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp?rev=a94b21d8308c#472
where we have to resolve font information before we can resolve a length.

I don't know which property this is for, but since it's StyleText()-calling-CalcLengthWith(), it must be one of the length-valued properties in StyleText().  (Probably line-height -- or text-indent, word-spacing, or letter-spacing. Of those, line-height is the most popularly-used, so that's probably the property involved here.)

SO: my first suggestion would be adjusting this content to *avoid* setting line-height (or whatever other StyleText property this is) to any em/ex/ch value -- maybe sticking with the default "normal" value (which AFAICT shouldn't require CalcLengthWith during a style-flush) and see if that improves things.

Comment 3

[Kevin Grandon :kgrandon]

Interestingly enough we never specified line-height until recently, and we currently only do so for a 4-column layout[1]. I'm not sure why we do this, but I suppose it would be worth an experiment to remove it and compare profiles.

[1] https://github.com/mozilla-b2g/gaia/blob/1eb81084fd398dc0fb3f4f7faab32da2aa3a99f4/shared/elements/gaia_grid/style.css#L92

Comment 4

[Daniel Holbert [:dholbert]]

dbaron suggests that we can speed up rem unit computation here by using the doc element's primary frame instead of re-resolving style here:

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp?rev=a94b21d8308c#450

I'll try doing that.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: patch to help w/ first restyle (spun off to bug 1049052): use root element's primary frame's style context

BenWa, can you try running your test with this patch applied and see if it helps?

Comment 6

[Benoit Girard (:BenWa)]

I'll try tomorrow.

Comment 7

[Benoit Girard (:BenWa)]

The first restyle is now 100ms shorter but the setup isn't scientific so I'd instead focus on how the sample code paths changed rather than time differences:

http://people.mozilla.org/~bgirard/cleopatra/#report=59542e6e923b46d54ed3e49055f9847fd2a178ab

Comment 8

[Benoit Girard (:BenWa)]

Is there enough information in this profile?

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Benoit Girard (:BenWa) from comment #7)
> The first restyle is now 100ms shorter but the setup isn't scientific so I'd
> instead focus on how the sample code paths changed rather than time
> differences:
[...]
(In reply to Benoit Girard (:BenWa) from comment #8)
> Is there enough information in this profile?

There's enough to determine that the patch helped, yeah:
 * In Comment 0's first restyle, nsStyleContext::StyleText() takes up 100 of the 270 samples.
 * In Comment 7's first restyle, nsStyleContext::StyleText() takes up 10 of the 166 samples.

So, that's a reduction from 37% of the restyle's time to 6% of the restyle's time.

As you may have noticed, I spun off bug 1049052 to land that patch, so that part is fixed in mozilla-central now. (though it only helps a bit in terms of the overall time in the complete profiles here.)

Comment 10

[Daniel Holbert [:dholbert]]

It looks like StyleText() made up about 30-40% of comment 0's second restyle, too (46/122 samples), vs. 11% of comment 7's second restyle (10/89 samples).  So the landed patch helps there as well.

Also, RE "StartBackgroundImageLoads is fairly big as well" from comment 0 -- that does seem to be the next big-ticket item (making up 47/166 samples in comment 7's profile), but it breaks down in a way that seems fairly reasonable, if you drill down. (Some time spent in nsContentUtils::LoadImage, divided between AddToLoadGroup and a few other things; and some time spent in nsContentUtils::CanLoadImage, which is split between several types of security checks. No one thing stands out as looking like needless string-copying or anything silly like that, AFAICT.)  This all seems like it could be consistent with just lots of images being loaded.

Also, the second restyle (in both profiles) seems like it's spending most of its time in a deeply-nested ::RestyleChildren stack, which makes me wonder slightly if bug 862276 might help (depending on what the style change is that's being flushed).  heycam would probably be a good person to follow up with.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Also, RE "StartBackgroundImageLoads is fairly big as well" from comment 0 --
> that does seem to be the next big-ticket item (making up 47/166 samples in
> comment 7's profile)

(Sorry, meant to say "...in the first restyle of comment 7's profile")

Comment 12

[Kevin Grandon :kgrandon]

Mass update: Resolve wontfix all issues with legacy homescreens.

As of 2.6 we have a new homescreen and having these issues open is confusing. All issues will block bug 1231115 so we can use that to re-visit any of these if needed.