System app is *still* abusing will-change

RESOLVED FIXED in Firefox OS v2.0

Status

Firefox OS
Gaia::System
P1
blocker
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: BenWa, Assigned: etienne)

Tracking

({perf, power, regression})

unspecified
2.0 S6 (18july)
ARM
Gonk (Firefox OS)
perf, power, regression
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [c=regression p= s= u=2.0] [systemsfe][MemShrink:P1])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
+++ This bug was initially created as a clone of Bug #1026240 +++

STR:
1) Install crystall skull
2) Turn on FPS counter

Notice the overfill counter is at 300 for a fullscreen webgl app. Remove the use of will-change in the system app I can get that number to 200 (still not ideal but better).

We can't have the task manager causing us 100 overfill just to speed up some effects. These devices don't have enough bandwidth and memory for this to be viable. Please remove this.

CSS:
/* Task Manager */
#screen.task-manager:not(.active-statusbar):not(.software-button-enabled) > #windows > .appWindow.in-task-manager,
#screen.task-manager:not(.active-statusbar):not(.software-button-enabled):not(:-moz-full-screen-ancestor) > #windows > .appWindow.in-task-manager:not(.fullscreen-app) {
  /* XXX sfoster - watch specificify here
     Position values are 1/2 because of scaling */
  top: calc(25% + 2.5rem);
  margin: 0 auto;
  overflow: hidden;
  transform-origin: 50% 0;
  transform: translateX(99.99%) scale(0.5, 0.5);
  will-change: transform;
}
(Reporter)

Comment 1

4 years ago
Created attachment 8450607 [details]
Displaylist
(Reporter)

Updated

4 years ago
blocking-b2g: --- → 2.0?

Updated

4 years ago
blocking-b2g: 2.0? → 2.0+
(Reporter)

Comment 2

4 years ago
I believe removing this was sufficient to get fullscreen webgl apps to start using HWC and remove the thebes layer but I'll have to confirm with my local patches poped.
(Assignee)

Comment 4

4 years ago
I don't think these rules should match anything while the task switcher isn't displayed :/
Sam can you confirm this?
Flags: needinfo?(sfoster)
(Reporter)

Comment 5

4 years ago
That's probably a good trade off. There's going to be a delay to prepare the layer tree to animate the app switching when we make it match again but it's better then permanently retaining that much graphical memory and paying additional cost when compositing each frame.
(Assignee)

Comment 6

4 years ago
Nope, the task manager is not at fault (removing the line mentioned in Comment 3 has no effect on the overfill), it's the edges, having a look.
Assignee: nobody → etienne
Flags: needinfo?(sfoster)
(Assignee)

Comment 7

4 years ago
Created attachment 8450902 [details] [review]
Gaia PR

This patch is bringing back the edge-candidate logic, not to change the will-change property itself since this still causes reflows, but making sure sheets that aren't accessible in 1 swipe aren't drawn.
Attachment #8450902 - Flags: review?(21)
(Assignee)

Comment 8

4 years ago
https://github.com/mozilla-b2g/gaia/commit/4980b4d47b943367b271f8de43f68b5320cec91b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Reporter)

Comment 9

4 years ago
I've updated gaia with the patch above and I'm still seeing will-change:transform, opacity matched when running Crystal Skull webapp app.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

4 years ago
(In reply to Benoit Girard (:BenWa) from comment #9)
> I've updated gaia with the patch above and I'm still seeing
> will-change:transform, opacity matched when running Crystal Skull webapp app.

Just want to clarify so we don't make another resolved/reopen trip.

- Should we make sure there's no will-change match when a app requests fullscreen?
- Should we make sure there's no will-change match when any app is open in the foreground?
- Should we make sure there's no will-change match period? and s/abusing/using/ the bug summary...
Flags: needinfo?(bgirard)
(Reporter)

Comment 11

4 years ago
(In reply to Etienne Segonzac (:etienne) from comment #10)
> (In reply to Benoit Girard (:BenWa) from comment #9)
> > I've updated gaia with the patch above and I'm still seeing
> > will-change:transform, opacity matched when running Crystal Skull webapp app.
> 
> Just want to clarify so we don't make another resolved/reopen trip.
> 
> - Should we make sure there's no will-change match when a app requests
> fullscreen?
> - Should we make sure there's no will-change match when any app is open in
> the foreground?
> - Should we make sure there's no will-change match period? and
> s/abusing/using/ the bug summary...

Having a will-change match is fine but having a will-change keep around more layers then we normally would is not (unless we're willing to consciously work in extra layers and a few MBs in our memory budget).

When does a will-change not incur extra layers? When that part of the document already provides active layer like the remote app iframe. However layers are not primed to optimize opacity and transform changes by default. So carefully requesting will-change on the right part of the subtree such that it match an active layer and exactly that will not incur extra layers or will not incur extra graphic memory usage.

What we need to do in this bug is make sure we're not incurring extra layers when we have an active app. Removing will-change entirely will remove the extra layers, but so will placing them within the constraints above which is admittedly more work. This has to be done by tracking the display list via a debug build + |user_pref("layout.display-list.dump", true);|.
Flags: needinfo?(bgirard)
Target Milestone: --- → 2.0 S6 (18july)
(Reporter)

Updated

4 years ago
Blocks: 1030608
Whiteboard: [c=regression p= s= u=2.0] [systemsfe] → [c=regression p= s= u=2.0] [systemsfe][MemShrink]
Whiteboard: [c=regression p= s= u=2.0] [systemsfe][MemShrink] → [c=regression p= s= u=2.0] [systemsfe][MemShrink:P1]
(Assignee)

Comment 12

4 years ago
Created attachment 8453169 [details] [diff] [review]
Follow up patch
(Assignee)

Comment 13

4 years ago
Here's a follow up patch, we still keep a layer ready for the previous and the next app in the stack, but just what's needed (ie. 1 ThebesLayer inside a ContainerLayer).

I tested with the following apps launched (it should be the worth case scenario):
dialer - message - contacts - crystal skull (displayed app) - settings

Display list before the patch
Painting --- retained layer tree:
ClientLayerManager (0xae7c8200)
  ClientContainerLayer (0xaf33f400) [visible=< (x=0, y=0, w=480, h=854); >] [opaqueContent] [metrics={ viewport=(x=0.000000, y=0.000000, w=320.000000, h=569.333313) cb=(x=0, y=0, w=480, h=854) viewportScroll=(x=0.000000, y=0.000000) displayport=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) critdp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) scrollableRect=(x=0.000000, y=0.000000, w=320.000000, h=569.333313) scrollId=0 }]
    ClientThebesLayer (0xb0bb5500) [clip=(x=0, y=0, w=0, h=0)] [not visible]
    ClientColorLayer (0xb0bb5a10) [not visible] [opaqueContent] [color=rgba(0, 0, 0, 1)]
    ClientContainerLayer (0xaf33f800) [clip=(x=0, y=0, w=480, h=854)] [visible=< (x=0, y=0, w=480, h=854); >] [opaqueContent]
      ClientThebesLayer (0xb0bb56b0) [visible=< (x=0, y=36, w=480, h=818); >] [opaqueContent] [valid=< (x=0, y=36, w=480, h=818); >]
      ClientContainerLayer (0xb1fe9400) [clip=(x=0, y=0, w=480, h=854)] [transform=[ 1 0; 0 1; -480 36; ]] [visible=< (x=0, y=0, w=480, h=818); >]
        ClientContainerLayer (0xb1fe9800) [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent]
          ClientThebesLayer (0xb0bb6af0) [clip=(x=0, y=0, w=0, h=0)] [not visible]
          ClientImageLayer (0xaf334b80) [clip=(x=0, y=0, w=480, h=818)] [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent]
          ClientContainerLayer (0xb1fea800) [clip=(x=0, y=0, w=480, h=818)] [visible=< (x=0, y=0, w=480, h=818); >]
            ClientThebesLayer (0xb2b84ca0) [visible=< (x=0, y=0, w=480, h=818); >] [valid=< (x=0, y=0, w=480, h=818); >]
      ClientContainerLayer (0xaf5c6800) [clip=(x=0, y=0, w=480, h=854)] [transform=[ 1 0; 0 1; 0 36; ]] [visible=< (x=0, y=0, w=480, h=818); >]
        ClientContainerLayer (0xaf5c6c00) [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent]
          ClientContainerLayer (0xaf57a800) [clip=(x=0, y=0, w=480, h=818)] [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent]
            ClientThebesLayer (0xaf9894e0) [clip=(x=0, y=0, w=0, h=0)] [not visible]
            ClientColorLayer (0xaf989690) [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent] [color=rgba(255, 255, 255, 1)]
          ClientRefLayer (0xaf57ac00) [clip=(x=0, y=0, w=480, h=818)] [visible=< (x=0, y=0, w=480, h=818); >] [id=7]
      ClientContainerLayer (0xaf5c7400) [clip=(x=0, y=0, w=480, h=854)] [transform=[ 1 0; 0 1; -480 36; ]] [visible=< (x=0, y=0, w=480, h=818); >]
        ClientContainerLayer (0xb2576800) [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent]
          ClientThebesLayer (0xaf989840) [clip=(x=0, y=0, w=0, h=0)] [not visible] [opaqueContent]
          ClientImageLayer (0xaf334840) [clip=(x=0, y=0, w=480, h=818)] [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent]
          ClientContainerLayer (0xb2b7b000) [clip=(x=0, y=0, w=480, h=818)] [visible=< (x=0, y=0, w=480, h=818); >]
            ClientThebesLayer (0xaf989330) [visible=< (x=0, y=0, w=480, h=818); >] [valid=< (x=0, y=0, w=480, h=818); >]
      ClientThebesLayer (0xb2b7ff00) [visible=< (x=0, y=0, w=480, h=36); >] [opaqueContent] [valid=< (x=0, y=0, w=480, h=36); >]


After the patch
Painting --- retained layer tree:
ClientLayerManager (0xaea4ac00)
  ClientContainerLayer (0xae7cf400) [visible=< (x=0, y=0, w=480, h=854); >] [opaqueContent] [metrics={ viewport=(x=0.000000, y=0.000000, w=320.000000, h=569.333313) cb=(x=0, y=0, w=480, h=854) viewportScroll=(x=0.000000, y=0.000000) displayport=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) critdp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) scrollableRect=(x=0.000000, y=0.000000, w=320.000000, h=569.333313) scrollId=0 }]
    ClientThebesLayer (0xaf5a9500) [clip=(x=0, y=0, w=0, h=0)] [not visible]
    ClientColorLayer (0xaf5a9a10) [not visible] [opaqueContent] [color=rgba(0, 0, 0, 1)]
    ClientContainerLayer (0xae7cf800) [clip=(x=0, y=0, w=480, h=854)] [visible=< (x=0, y=0, w=480, h=854); >] [opaqueContent]
      ClientThebesLayer (0xaf5a96b0) [visible=< (x=0, y=36, w=480, h=818); >] [opaqueContent] [valid=< (x=0, y=36, w=480, h=818); >]
      ClientContainerLayer (0xb16aa000) [clip=(x=0, y=0, w=480, h=854)] [transform=[ 1 0; 0 1; -480 36; ]] [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent]
        ClientThebesLayer (0xaf5a74f0) [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent] [valid=< (x=0, y=0, w=480, h=818); >]
      ClientContainerLayer (0xaf5b5c00) [clip=(x=0, y=0, w=480, h=854)] [transform=[ 1 0; 0 1; 0 36; ]] [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent]
        ClientContainerLayer (0xb16a4400) [clip=(x=0, y=0, w=480, h=818)] [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent]
          ClientThebesLayer (0xb2b81d60) [clip=(x=0, y=0, w=0, h=0)] [not visible]
          ClientColorLayer (0xb2b84af0) [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent] [color=rgba(255, 255, 255, 1)]
        ClientRefLayer (0xb16aa800) [clip=(x=0, y=0, w=480, h=818)] [visible=< (x=0, y=0, w=480, h=818); >] [id=7]
      ClientContainerLayer (0xae969400) [clip=(x=0, y=0, w=480, h=854)] [transform=[ 1 0; 0 1; -480 36; ]] [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent]
        ClientThebesLayer (0xaf5a9d70) [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent] [valid=< (x=0, y=0, w=480, h=818); >]
      ClientThebesLayer (0xaf5a9f20) [visible=< (x=0, y=0, w=480, h=36); >] [opaqueContent] [valid=< (x=0, y=0, w=480, h=36); >]


Is this an acceptable compromise?
We can go further and remove the edge-candidate logic but it's will degrade the user experience (at the beginning of a gesture, or when swiping quickly between many apps).
Flags: needinfo?(bgirard)
When comparing user experiences, make sure we're testing with 256mb Flame (273mb configuration.)
(Reporter)

Comment 15

4 years ago
The after tree still has 3 fullscreen thebes layers which is still very inefficient. We must only have the status bar providing a thebes layer otherwise we're going to regress Hardware composer.
Flags: needinfo?(bgirard)
(Reporter)

Comment 16

4 years ago
bug 1027231 has landed, make sure to test with that patch when looking at a layer tree.
(Assignee)

Comment 17

4 years ago
Thanks for the pointers, I'll make a new patch today and test it with a 273MB flame.
(Assignee)

Comment 18

4 years ago
Created attachment 8453821 [details] [diff] [review]
Follow up patch v2

Here's the new display list dump (same setup as before):

Painting --- retained layer tree:
ClientLayerManager (0xaecdd300)
  ClientContainerLayer (0xaf098400) [visible=< (x=0, y=0, w=480, h=854); >] [opaqueContent] [metrics={ cb=(x=0.000000, y=0.000000, w=480.000000, h=854.000000) sr=(x=0.000000, y=0.000000, w=320.000000, h=569.333313) s=(x=0.000000, y=0.000000) dp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) cdp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000) z=1.500 }]
    ClientThebesLayer (0xaf484930) [clip=(x=0, y=0, w=0, h=0)] [not visible]
    ClientColorLayer (0xaf484e40) [not visible] [opaqueContent] [color=rgba(0, 0, 0, 1)]
    ClientContainerLayer (0xaf098800) [clip=(x=0, y=0, w=480, h=854)] [visible=< (x=0, y=0, w=480, h=854); >] [opaqueContent]
      ClientThebesLayer (0xaf484ae0) [not visible] [opaqueContent] [valid=< (x=0, y=36, w=480, h=818); >]
      ClientContainerLayer (0xad071c00) [clip=(x=0, y=0, w=480, h=854)] [transform=[ 1 0; 0 1; 0 36; ]] [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent]
        ClientThebesLayer (0xaca8b5c0) [clip=(x=0, y=0, w=0, h=0)] [not visible]
        ClientColorLayer (0xaca8b770) [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent] [color=rgba(0, 0, 0, 1)]
      ClientContainerLayer (0xaf003000) [clip=(x=0, y=0, w=480, h=854)] [transform=[ 1 0; 0 1; 0 36; ]] [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent]
        ClientContainerLayer (0xaf788000) [clip=(x=0, y=0, w=480, h=818)] [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent]
          ClientThebesLayer (0xaca8a9f0) [clip=(x=0, y=0, w=0, h=0)] [not visible]
          ClientColorLayer (0xaca8bad0) [visible=< (x=0, y=0, w=480, h=818); >] [opaqueContent] [color=rgba(255, 255, 255, 1)]
        ClientRefLayer (0xaf78ac00) [clip=(x=0, y=0, w=480, h=818)] [visible=< (x=0, y=0, w=480, h=818); >] [id=7]
      ClientThebesLayer (0xaca8b260) [visible=< (x=0, y=0, w=480, h=36); >] [opaqueContent] [valid=< (x=0, y=0, w=480, h=36); >]


We're now using the hardware composer while displaying crystall skull (and the dialer, and messages and contacts BTW).

We reflow once per app change (ie. only once if you swipe multiple times), but not during the gestures, and there's no relayerization occurring while a finger is in contact with the screen either.
Attachment #8453169 - Attachment is obsolete: true
Attachment #8453821 - Flags: review?(21)
Attachment #8453821 - Flags: feedback?(bgirard)
(Reporter)

Comment 19

4 years ago
Comment on attachment 8453821 [details] [diff] [review]
Follow up patch v2

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

The layer tree looks great!

I don't know enough about the app to know when what selector is matching is matching what to do a careful review. Keep in mind that if the tree that we're inferring is in-actived too quickly while the user is interacting with the task switcher, that a good use of will-change is to momentarily activate it with a timeout on the element.
Attachment #8453821 - Flags: feedback?(bgirard) → feedback+
Comment on attachment 8453821 [details] [diff] [review]
Follow up patch v2

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

LGTM.
Attachment #8453821 - Flags: review?(21) → review+
(Assignee)

Comment 21

4 years ago
https://github.com/mozilla-b2g/gaia/commit/b0037a01a564b3850434ccbdce82f31259abb557
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1035266
(Assignee)

Updated

4 years ago
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → fixed
See Also: → bug 1043870
Unable to test, we do not have access to crystal skull webapp through marketplace or crystalskull.gaiamobile.org
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+][QAnalyst-verify-]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.