Closed Bug 1034347 Opened 10 years ago Closed 10 years ago

System app is *still* abusing will-change

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

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

RESOLVED FIXED
2.0 S6 (18july)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: BenWa, Assigned: etienne)

References

Details

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

Attachments

(3 files, 1 obsolete file)

+++ 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;
}
Attached file Displaylist
blocking-b2g: --- → 2.0?
blocking-b2g: 2.0? → 2.0+
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.
I don't think these rules should match anything while the task switcher isn't displayed :/
Sam can you confirm this?
Flags: needinfo?(sfoster)
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.
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)
Attached file 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)
https://github.com/mozilla-b2g/gaia/commit/4980b4d47b943367b271f8de43f68b5320cec91b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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 → ---
(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)
(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)
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]
Attached patch Follow up patch (obsolete) — Splinter Review
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.)
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)
bug 1027231 has landed, make sure to test with that patch when looking at a layer tree.
Thanks for the pointers, I'll make a new patch today and test it with a 273MB flame.
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)
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+
https://github.com/mozilla-b2g/gaia/commit/b0037a01a564b3850434ccbdce82f31259abb557
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
See Also: → 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.

Attachment

General

Created:
Updated:
Size: