930587 - [Homescreen] Style manipulation is expensive

Status: RESOLVED FIXED

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

Description

[Benoit Girard (:BenWa)]

Attachment: test to stop the restyle

The homescreen uses a data-transitioning CSS selector. Toggling this needs to update the style of the children of that which currently is applied on the body element:
body[data-transitioning] .apps > div

This causes a full style flush for the document which takes over 16ms. We can't afford to this at all during the start touch event.

I also noticed that the Homescreen changes the visibility attribute. It appears to be done for performance however this prevent optimizations like pre-rendering from working properly. We need to tweak the app to force active layers and pre-render the other page. This way when a pan start we can start the animation immediatly and not have to perform any expensive style flush and paints.

The delay for the first frame is 100ms: 30ms for layout and 70ms for painting.

Comment 1

[Benoit Girard (:BenWa)]

Here's a profile:
http://people.mozilla.org/~bgirard/cleopatra/#report=1a16c320066e2be47f40d07f2c6eb96f3f30a2de

Sorry there's a lot of advanced features turned on so it's difficult to read.

Comment 2

[Francis Lee [:frlee]]

this issue blocks 920921, according to triage result, mark it koi+

Comment 3

[Peter Chang[:pchang]]

(In reply to Benoit Girard (:BenWa) from comment #0)
> Created attachment 821753 [details] [diff] [review]
> test to stop the restyle
> 
> The homescreen uses a data-transitioning CSS selector. Toggling this needs
> to update the style of the children of that which currently is applied on
> the body element:
> body[data-transitioning] .apps > div
> 
> This causes a full style flush for the document which takes over 16ms. We
> can't afford to this at all during the start touch event.
> 
> I also noticed that the Homescreen changes the visibility attribute. It
> appears to be done for performance however this prevent optimizations like
> pre-rendering from working properly. We need to tweak the app to force
> active layers and pre-render the other page. This way when a pan start we

I tried to set layer.force-active flag on b2g, but I still could see the layer recreation when scroll to new pages. Not sure it is the right behavior or not.

I got a question here. If homescreen didn't change the visibility attribute, the layer which contains one page and is out of screen will be kept or removed because out of screen.

Comment 4

[Peter Chang[:pchang]]

Benwa, did you see buffer re-allocation when scroll homescreen to new page? I still saw it after applying your test patch.

Comment 5

[Benoit Girard (:BenWa)]

(In reply to peter chang[:pchang] from comment #3)
> I tried to set layer.force-active flag on b2g, but I still could see the
> layer recreation when scroll to new pages. Not sure it is the right behavior
> or not.
> 

I would expect it to work but I don't know for sure.

> I got a question here. If homescreen didn't change the visibility attribute,
> the layer which contains one page and is out of screen will be kept or
> removed because out of screen.

I asked roc and he said if the layer is active and within 32 pixels of the screen it should be pre-rendered. We need to figure out why it doesn't happen like according to plan.

The pre-rendering isn't working properly but I don't know why. We will need a debugging session to see what decides to recreate or invalidate the buffer.

Comment 6

[u459114]

Peter,
nsDisplayTransform::ShouldPrerenderTransformedContent is the function to determine whether prerender a layer. FYI
http://dxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#l4197

Comment 7

[Peter Chang[:pchang]]

(In reply to Benoit Girard (:BenWa) from comment #5)
> (In reply to peter chang[:pchang] from comment #3)
> > I tried to set layer.force-active flag on b2g, but I still could see the
> I asked roc and he said if the layer is active and within 32 pixels of the
> screen it should be pre-rendered. We need to figure out why it doesn't
> happen like according to plan.
> 
> The pre-rendering isn't working properly but I don't know why. We will need
> a debugging session to see what decides to recreate or invalidate the buffer.

To make pre-rendering work and no gralloc re-allocation, we need two things.
a. Align current page coordination with previous and next pages, like page1(-320, 0) page2(0,0) page3 (320, 0) if screen width is 320.
b. Set transfrom-style:preserve-3d and call transform: rotateY(1deg)

If we modified homescreen with above item b, the ShouldBuildLayerEvenIfInvisible(pre-render) will become true. And then it will construct a layer for current frame.

http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#2123

[Pre-render]
The following lines are related code for pre-render condition check.
ShouldBuildLayerEvenIfInvisible(FrameLayerBuild.cpp)
-> nsDisplayTransform::ShouldPrerenderTransformedContent(nsDisplayList.cpp)
-> nsIFrame::AreLayersMarkedActive(nsFrame.cpp)
-> Check Preserve3D property

http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#2109
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#4209
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#4675

Comment 8

[Peter Chang[:pchang]]

https://github.com/cctuan/gaia/commits/test222

we had co-worked with TPE gaia George and use above github for testing.

Comment 9

[Mike Lee [:mlee]]

Updating Target Milestone for FxOS Perf koi+'s.

Comment 10

[u459114]

(In reply to peter chang[:pchang] from comment #7)
> (In reply to Benoit Girard (:BenWa) from comment #5)
> > (In reply to peter chang[:pchang] from comment #3)
> > > I tried to set layer.force-active flag on b2g, but I still could see the
> > I asked roc and he said if the layer is active and within 32 pixels of the
> > screen it should be pre-rendered. We need to figure out why it doesn't
> > happen like according to plan.
> > 
> > The pre-rendering isn't working properly but I don't know why. We will need
> > a debugging session to see what decides to recreate or invalidate the buffer.
> 
> To make pre-rendering work and no gralloc re-allocation, we need two things.
> a. Align current page coordination with previous and next pages, like
> page1(-320, 0) page2(0,0) page3 (320, 0) if screen width is 320.
> b. Set transfrom-style:preserve-3d and call transform: rotateY(1deg)
> 
> If we modified homescreen with above item b, the
> ShouldBuildLayerEvenIfInvisible(pre-render) will become true. And then it
> will construct a layer for current frame.
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.
> cpp#2123
> 
> [Pre-render]
> The following lines are related code for pre-render condition check.
> ShouldBuildLayerEvenIfInvisible(FrameLayerBuild.cpp)
> -> nsDisplayTransform::ShouldPrerenderTransformedContent(nsDisplayList.cpp)
> -> nsIFrame::AreLayersMarkedActive(nsFrame.cpp)
> -> Check Preserve3D property
> 
> http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.
> cpp#2109
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.
> cpp#4209
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#4675
In, ContainerState::ProcessDisplayItems
http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#2109

if (itemVisibleRect.IsEmpty() &&
   !item->ShouldBuildLayerEvenIfInvisible(mBuilder)) {
   continue;
}
Which means if you want a layer be pre-rendering and reserve only if 
1. That item intersect wit view port
2. A item which is outside of viewport, it need to pass test in ShouldBuildLayerEvenIfInvisible.

I think we can use condition of #1, make left and right page be intersect with viewport by 1 pixel.

Comment 11

[Mike Lee [:mlee]]

BenWa, I'm setting you as the assignee for this bugs since you've submitted a path. If you're not the right person for this please reply saying so.

Comment 12

[Benoit Girard (:BenWa)]

I'd rather pass this on to the app developers since they know this code better. There's other platforms issues that need to be fixed for the homescreen and this is purely a problem on the app side.

Looks like Eitan wrote the initial code. Can you look into reworking the style change? I'd imagine setting the style directly on what needs it would solve the problem. Or avoiding this entirely would help the time to the first frame on touch down. For more complex cost questions of manipulating styles needinfo dholbert.

Comment 13

[Benoit Girard (:BenWa)]

This is great info CJKu and pchang. Let's move that discussion to bug 931262 and leave this bug strictly as an app issue to rewrote how it manipulate styles to avoid an expensive flush.

Comment 14

[Cristian Rodriguez (:crdlc)]

inline

(In reply to Benoit Girard (:BenWa) from comment #0)
> Created attachment 821753 [details] [diff] [review]
> test to stop the restyle
> 
> The homescreen uses a data-transitioning CSS selector. Toggling this needs
> to update the style of the children of that which currently is applied on
> the body element:
> body[data-transitioning] .apps > div
> 
> This causes a full style flush for the document which takes over 16ms. We
> can't afford to this at all during the start touch event.

1. This rule could be avoided by means of body[data-mode='edit'] (it is used to stop the bouncing effect of icons while panning)
body[data-mode='edit'][data-transitioning] ol > li > div > img
https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/style/dragdrop.css#L55

2. Read about accessibility, see below
https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/style/grid.css#L29

3. This rule could be removed because this stops the installing animation around an icon while panning (edge case)
https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/style/grid.css#L155

> 
> I also noticed that the Homescreen changes the visibility attribute. It
> appears to be done for performance however this prevent optimizations like
> pre-rendering from working properly. We need to tweak the app to force
> active layers and pre-render the other page. This way when a pan start we
> can start the animation immediatly and not have to perform any expensive
> style flush and paints.

The visibility changes were introduced for accessibility https://github.com/mozilla-b2g/gaia/commit/08ba523403475799861b06166391811226c8a0c8. From my point of view, all stuffs about visibility could be removed thought :)

> 
> The delay for the first frame is 100ms: 30ms for layout and 70ms for
> painting.

Comment 15

[Eitan Isaacson [:eeejay]]

(In reply to Benoit Girard (:BenWa) from comment #12)
> Looks like Eitan wrote the initial code. Can you look into reworking the
> style change? I'd imagine setting the style directly on what needs it would
> solve the problem. Or avoiding this entirely would help the time to the
> first frame on touch down. For more complex cost questions of manipulating
> styles

The two alternatives would be (a) style the actual list items as invisible, would that help? or (b) use aria-hidden, which requires some js hoopla and has the danger of decoupling from the actual visual state.

Comment 16

[Eitan Isaacson [:eeejay]]

Attachment: Style individual app icons instead of full pages.

Comment 17

[Benoit Girard (:BenWa)]

Comment on attachment 826828 [details] [diff] [review]
Style individual app icons instead of full pages.

Bouncing this to dholbert from lack of CSS knowledge.

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 826828 [details] [diff] [review]
Style individual app icons instead of full pages.

At a purely "is this valid CSS" level, this looks fine.

(IIUC, this is styling the app icons to be invisible, except for those on the current page, and also except for when the user is swiping the homescreen side to side.)

This should be an improvement, from the perspective of making style changes more granular. I'm not sure it addresses the issue that BenWa brought up at the end of comment 0, though. (BenWa said there that visibility:hidden prevents "optimizations like pre-rendering from working properly") BenWa, do you know (or, can you test) to see if this helps at all?

From that standpoint, it may end up being better to drop these "visibility" declarations altogether and using aria-hidden -- Eitan's option (b).

Comment 19

[Benoit Girard (:BenWa)]

I don't know much about accessibility . I'm guessing aria-hidden is something to tell the screen reader to ignore it. For non accessibility performance making something that's near offscreen hidden will make performance worse because we will make the wrong decisions and incur more work and/or miss optimization.

I guess the question then is how to best have accessibility deal with things that are offscreen? Then I don't know. Ideally it can perform visibility testing properly but in the worse case if aria-hidden has no side effect for non accessibility it should fix this performance problem.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

It looks like Eitan took this.  We just want to make sure the assignee is set so we can see if any koi+ bugs aren't covered.  Eitan please fix if you're not actively working this.

Comment 21

[Eitan Isaacson [:eeejay]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13439

Pointer to Github pull-request

Comment 22

[Cristian Rodriguez (:crdlc)]

Comment on attachment 828187 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13439

Great work! Thanks a lot my friend!

Comment 23

[Cristian Rodriguez (:crdlc)]

https://github.com/mozilla-b2g/gaia/commit/e8fbba0c9716d3a4454ed171ca7b4410b9c4bd9d

Comment 24

[Benoit Girard (:BenWa)]

Thanks Eitan.

I think this will have some improvements but I still expect to see some big Style blocks showing up in profiles but this means we can move into the next issue!

Comment 25

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted e8fbba0c9716d3a4454ed171ca7b4410b9c4bd9d to:
v1.2: 244ec6e3afa470667c8dfd63dc8cf7a0c1fd6590