Reduce overall overpaint

RESOLVED FIXED in 2.1 S3 (29aug)

Status

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

unspecified
2.1 S3 (29aug)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:-)

Details

Attachments

(10 attachments, 4 obsolete attachments)

996 bytes, patch
alive
: review+
Details | Diff | Splinter Review
353.02 KB, image/png
Details
422.17 KB, image/png
Details
482 bytes, text/html
Details
6.87 KB, text/plain
Details
5.57 KB, text/plain
Details
6.27 KB, text/plain
Details
4.62 KB, text/plain
Details
769 bytes, patch
fabrice
: review+
etienne
: feedback+
Details | Diff | Splinter Review
2.65 KB, patch
etienne
: review+
Details | Diff | Splinter Review
Posted patch bug.less.overpaint.patch (obsolete) — Splinter Review
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.
Attachment #8466783 - Flags: feedback?(bgirard)
Posted patch bug.less.overpaint.patch (obsolete) — Splinter Review
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.
Attachment #8466783 - Attachment is obsolete: true
Attachment #8466783 - Flags: feedback?(bgirard)
Attachment #8466784 - Flags: feedback?(bgirard)
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.
(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.
Posted patch bug.less.overpaint.patch (obsolete) — Splinter Review
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.
Attachment #8466784 - Attachment is obsolete: true
Attachment #8466784 - Flags: feedback?(bgirard)
Attachment #8466804 - Flags: feedback?
(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.
Posted patch bug.less.overpaint.patch (obsolete) — Splinter Review
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!
Attachment #8466804 - Attachment is obsolete: true
Attachment #8466804 - Flags: feedback?
Attachment #8466977 - Flags: feedback?(bgirard)
(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).
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?
Flags: needinfo?(tnikkel)
(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.
(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.
I'm testing this on my Nexus S, and for now it feels like there's a nice general responsiveness improvement :)
Flags: needinfo?(lissyx+mozillians)
Posted patch system.patchSplinter Review
Let's just replace the offending rules |translateX(0)| by |transform: none;|.
Attachment #8466977 - Attachment is obsolete: true
Attachment #8466977 - Flags: feedback?(bgirard)
Attachment #8469943 - Flags: review?(alive)
Attachment #8469943 - Flags: review?(alive) → review+
[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.
blocking-b2g: --- → 2.0?
Posted image 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
Flags: needinfo?(lissyx+mozillians)
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.
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.
blocking-b2g: 2.0? → -
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 :/
(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).
Flags: needinfo?(roc)
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?
Flags: needinfo?(roc)
Posted file 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.
Flags: needinfo?(mchang)
Overfill ratio of 319.
Flags: needinfo?(mchang)
Overfill ratio of 314.
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.
Attachment #8474604 - Flags: review?(etienne)
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.
Attachment #8474609 - Flags: review?(etienne)
Attachment #8474609 - Flags: review?(etienne) → review+
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?
(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 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 :)
Attachment #8474604 - Flags: review?(fabrice)
Attachment #8474604 - Flags: review?(etienne)
Attachment #8474604 - Flags: feedback+
Attachment #8474604 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/5a9eeb0c201f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S3 (29aug)
Depends on: 1099272
Depends on: 1155785
Flags: needinfo?(tnikkel)
You need to log in before you can comment on or make changes to this bug.