1048009 - Reduce overall overpaint

Status: RESOLVED FIXED

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

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug.less.overpaint.patch

While looking at bug 1047390 I noticed that the overall overpaint was way too big. The patch in bug 1047390 fixes the regression that has been introduced by adding a new scrollable div for system browser frame.

That said we can reduce the overall overpaint by adding all the iframes that are not actively painted on the screen. This poc tries to hide as much of them as possible. There is bug with the homescreen when you start the device, but otherwise on an application like Settings I see the overpaint going from 484 to 384 when not panning, and from 514 to 414 while panning.

There is also a small regression as edge-gesture expect the background behind it to be black, but this should be easily fixable.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug.less.overpaint.patch

One thing we can do is probably to set the a class on the #screen element when an application is active and not the homescreen.

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 8466784 [details] [diff] [review]
bug.less.overpaint.patch

I don't know enough about the system app to really know when the selectors will match. But my initial gut feeling is to wonder why layout can't figure out that these elements are not visible on its own.

When you say 'not actively rendered on the screen', do you mean that the content is occluded by something else? If so I would instead look at the element(s) above it and making sure that they are understood to be opaque when possible. Once something is understood to be opaque and there's no will-change or any content considered 'active', elements below should not be painted. Not only will it save overdraw but it will speed up rendering.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Benoit Girard (:BenWa) from comment #2)
> Comment on attachment 8466784 [details] [diff] [review]
> bug.less.overpaint.patch
> 
> I don't know enough about the system app to really know when the selectors
> will match. But my initial gut feeling is to wonder why layout can't figure
> out that these elements are not visible on its own.
> 

I can't really tell which windows are visible from layout. But as a guess I feel like the homescreen window is considered visible (it is under current app window) and other windows that are opened and are moved on the left/right sides with translateX.

All those selectors are in order to hide application window, unless it is going to be transitioned from one state to an other one. But otherwise only the current window is set to display: block.

> When you say 'not actively rendered on the screen', do you mean that the
> content is occluded by something else? 

At least I can't visually see the content with my eyes, as it is offscreen, or under the current app.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug.less.overpaint.patch

I also spent some additional time on the poc. Here is the best I can do with those very long selectors.

Overpainting goes from 484 to 284 when not panning, and from 514 to 314 when panning in the Settings app.

Benoit, I'm more or less happy with those 2 lines of CSS for the moment. But I would like to make sure there is no big platform issue that needs to be fixed instead of going this way.
That's more or less why I f? you on this poc.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #4)
> Created attachment 8466804 [details] [diff] [review]
> bug.less.overpaint.patch
> 
> I also spent some additional time on the poc. Here is the best I can do with
> those very long selectors.
> 
> Overpainting goes from 484 to 284 when not panning, and from 514 to 314 when
> panning in the Settings app.
> 
> Benoit, I'm more or less happy with those 2 lines of CSS for the moment. But
> I would like to make sure there is no big platform issue that needs to be
> fixed instead of going this way.
> That's more or less why I f? you on this poc.

That's probably the best we can do from the system app side. With this poc I see the template app overpaint at 100 when I launch it, while it set to 300 without it.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug.less.overpaint.patch

Wow. It seems like my theory was wrong and this is not related to side windows or the homescreen beeing beneath. But applying a transform: translateX(0) to the current active/displayed app makes the overpaint goes up of 200!

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #6)
> Created attachment 8466977 [details] [diff] [review]
> bug.less.overpaint.patch
> 
> Wow. It seems like my theory was wrong and this is not related to side
> windows or the homescreen beeing beneath. But applying a transform:
> translateX(0) to the current active/displayed app makes the overpaint goes
> up of 200!

I played with some apps to see if they are affected by |transform: translateX(0)| as well.
It seems like the call log in the dialer app, the messages app, the contacts app, the settings app, the email app, the contacts app and the music app use |transform: translateX(0)| by default and it results into a increased overpaint.

In order to check I just used the about:app-manager and deactivate the rule on the main container of the app, which is often transformed at 0 in order to moved it afterward. Just deactivating the rule results into decreasing the general overpaint.

Benoit, is it expected or is there anything we can do on the platform side to fix that for the case of |transform: translateX(0)| and save a lot of overpaint for all apps in once ? (between 200 and 300 for all apps).

Comment 8

[Benoit Girard (:BenWa)]

My gut feeling is that adding more CSS is the wrong fix. I'd be much more favorable to use an approach that remove the nodes entirely from the DOM since this would make reflows, restyle and visibility calculations faster. But this isn't my area of expertise so I'm going to defer to layout:

tn, what do you think of the above patch to remove overdraw. Do you know why translateX elements would contribute to overdraw if they aren't active?

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Benoit Girard (:BenWa) from comment #8)
> My gut feeling is that adding more CSS is the wrong fix. 

I agree. And I would rather prefer a fix from on the platform side.

> I'd be much more
> favorable to use an approach that remove the nodes entirely from the DOM
> since this would make reflows, restyle and visibility calculations faster.
> But this isn't my area of expertise so I'm going to defer to layout:
> 

Well, the element were translateX(0) applies is the current displayed window on the screen. So it can't be removed.

Same for apps, the translateX(0) is for the current displayed element on the screen.

> tn, what do you think of the above patch to remove overdraw. Do you know why
> translateX elements would contribute to overdraw if they aren't active?

Based on the previous comment, I think the elements are 'active'. For some cases translateX(0) is here because at some point the element can be translate to 100% or -100% with an animation.

Comment 10

[Benoit Girard (:BenWa)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #9)
> (In reply to Benoit Girard (:BenWa) from comment #8)
> > My gut feeling is that adding more CSS is the wrong fix. 
> 
> I agree. And I would rather prefer a fix from on the platform side.

If there's a platform issue let's get a reduced test case up and reason about the optimization we can't infer. I have a lot of trouble reasoning about what is going on since the gaia app pages are quite complex both DOM and CSS wise. If the element is marked as transform then I believe that very heavily complicate culling.

> 
> Same for apps, the translateX(0) is for the current displayed element on the
> screen.
> 
> > tn, what do you think of the above patch to remove overdraw. Do you know why
> > translateX elements would contribute to overdraw if they aren't active?
> 
> Based on the previous comment, I think the elements are 'active'. For some
> cases translateX(0) is here because at some point the element can be
> translate to 100% or -100% with an animation.

Can't we come up with simpler selectors rather then adding more complex !important selector to work around the issue? For the case where we have translateX(0) it would be better if no CSS translate matched at all.

Comment 11

[:gerard-majax]

I'm testing this on my Nexus S, and for now it feels like there's a nice general responsiveness improvement :)

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: system.patch

Let's just replace the offending rules |translateX(0)| by |transform: none;|.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

[Blocking Requested - why for this release]:

Somes measurements from Alexandre Lissyx told us that this patch may also helps to reduce some ion allocations. For this reason, I'm asking to backport that on 2.0.

This patch is pretty safe, and is basically 2 lines of CSS. Pretty easy to backout if we can see any regressions, which is unlikely thought.

I also think all the patches that blocks this one, which also are CSS-only changes would be good to take for 2.0. Particularly the one that helps reduce the overpaint of the homescreen.

I let Alexandre post any memory charts if needed.

Comment 14

[:gerard-majax]

Attachment: ion_bug1048009.png

Attached are the plots Vivien was referring about. It shows that when we apply the patch, the local maximums of mapped ION memory on main process is lower in generalt, and by about 3MB.

This was on a master userdebug build for Flame, with default user apps and 3 icons layout. When tracking memory usage, I did the following steps:
 - launch homescreen
 - unlock pin
 - scroll up/down a couple of times on homescreen
 - launch settings, scroll down, open developper menu, go back
 - go to homescreen, launch dialer
 - press power button

Comment 15

[:gerard-majax]

Attachment: ion_bug1048009_and_depends.png

Impact of bug 1048009 and its dependencies on ION memory use: we still see the win previously documented on the b2g process, but the impact for other processes is not visible.

Comment 16

[Dietrich Ayala (:dietrich)]

Discussed in triage: Does seem low risk, but there's no way to know until it lands, and 2.0 stability is priority #1 right now.

Please renominate after it lands on master and is known to be truly safe.

Comment 17

[Etienne Segonzac (:etienne)]

Comment on attachment 8469943 [details] [diff] [review]
system.patch

Review of attachment 8469943 [details] [diff] [review]:
-----------------------------------------------------------------

without the transform we're getting a reflow at the beginning of each edge gesture :/

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Etienne Segonzac (:etienne) from comment #17)
> Comment on attachment 8469943 [details] [diff] [review]
> system.patch
> 
> Review of attachment 8469943 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> without the transform we're getting a reflow at the beginning of each edge
> gesture :/

I know :(

My current guess is using |transform: none| remove a nsDisplayTransform item from the display list. Changing it to any other value force the creation of a nsDisplayTransform, and so rebuild the display list.

Sadly I don't think there is any CSS tricks that will prevent that. I don't know enough of the layout/gfx code to know if there is a platform way to keep the transform: translateX(0), avoid the overpaint, and avoid the reflow as well.

Let needinfo'ed roc to see if there is a possible way to do that (and my previous theory about the display list changes can be wrong as well).

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

By overpainting you mean overdrawing in the compositor, right? Or do you mean something else?

A couple of things I don't understand here:
-- Having a static translateX(0) transform should not trigger creation of an active transformed layer. translateX(0) vs none should not affect what gets composited.
-- Adding and removing transforms should not cause reflows in common cases. If the transformed element has positioned descendants whose abs-pos or fixed-pos container switches from some ancestor of the transformed element to the transformed element, we will have to recreate frames and reflow them. Is that happening here?

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: transform.html

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> By overpainting you mean overdrawing in the compositor, right? Or do you
> mean something else?
> 

Since the 2 terms pretty much means the same thing for me, I would say 'yes'.

Just in case, what I refer to is the number that appears in the the 3rd pink rectangle on the top-left of this screenshot: http://vingtetun.org/tmp/statusbar-filter.png, which is displayed when the preference |layers.acceleration.draw-fps| is set to true.


> A couple of things I don't understand here:
> -- Having a static translateX(0) transform should not trigger creation of an
> active transformed layer. translateX(0) vs none should not affect what gets
> composited.

Does 2 layer tree dumps with |transform: translateX(0);| and one with |transform: none;| may help to see if the an active transformed layer is created ?

I'm on PTO, let's see if Mason has some free time for those :)

> -- Adding and removing transforms should not cause reflows in common cases.
> If the transformed element has positioned descendants whose abs-pos or
> fixed-pos container switches from some ancestor of the transformed element
> to the transformed element, we will have to recreate frames and reflow them.
> Is that happening here?

I may have misunderstood the question. In case I got it right:

The transformed element should already be the container of the positioned descendants. Mostly because the transformed element has a |position:absolute| rule set at https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/window_layout.css#L42

Also I made a small testcase (see attachment), which toggle between |transform: translateX(0)| and |transform: none;| based on the CSS rules mentioned above.

I can see a reflow if the transformed element has an |overflow: hidden;| rule, but none otherwise.
I removed all the other rules as they does not seems to affect the testcase.

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1050727#c4 it seems like it may remove one layer.

Comment 22

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree - Settings - Master

Overfill of 514.

Comment 23

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree - Homescreen - Master

Overfill ratio of 319.

Comment 24

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree - Settings - With Patch

Overfill ratio of 314.

Comment 25

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree - Homescreen - With Patch

Overfill ratio of 315.

Comment 26

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug1041576.workaround.patch

In order to avoid the reflow for edge gestures I add to be a bit more agressive. So this patch will be splitted in 2 patches.

Comment 27

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: less.overpaint.all.patch

So I basically remove the |overflow: hidden| rule on .appWindow, while I kept it for the homescreen (because of the scaling transition).

I also add to move one of the scrollbar workaround one level up (from gaia/window_layouy.css to b2g/content.css, because I need access to privileged CSS to hide the scrollbar instead of having a wider container.

Comment 28

[Etienne Segonzac (:etienne)]

Comment on attachment 8474604 [details] [diff] [review]
bug1041576.workaround.patch

Review of attachment 8474604 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a bit uneasy reviewing this...
Is there a way to specify this in a way less dependent of the build system?

Comment 29

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Etienne Segonzac (:etienne) from comment #28)
> Comment on attachment 8474604 [details] [diff] [review]
> bug1041576.workaround.patch
> 
> Review of attachment 8474604 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a bit uneasy reviewing this...
> Is there a way to specify this in a way less dependent of the build system?

Nothing that I can imagine on top of my head without exposing this workaround to web content. Please note that this line of css will just get removed once the mentioned platform issue is resolved.

Comment 30

[Etienne Segonzac (:etienne)]

Comment on attachment 8474604 [details] [diff] [review]
bug1041576.workaround.patch

Review of attachment 8474604 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, it's still worth it while waiting for the underlying bug to get fixed.
Let's get it stamped by a real b2g/ peer still :)

Comment 31

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Pushed the b2g/ part.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a9eeb0c201f

Comment 32

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Gaia part: https://github.com/mozilla-b2g/gaia/commit/1166fa5ebeeaac147e97e4cf71b784ddbc7cd91d

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5a9eeb0c201f