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)
Tracking
(blocking-b2g:2.0+, 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;
}
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
blocking-b2g: --- → 2.0?
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0+
Reporter | ||
Comment 2•10 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.
Comment 3•10 years ago
|
||
Here is the file for anyone looking for it: https://github.com/mozilla-b2g/gaia/blob/24bb25511cfd657a11cc801dbbc14fac3bdaf4f7/apps/system/style/window.css#L669
Assignee | ||
Comment 4•10 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•10 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•10 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•10 years ago
|
||
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)
Attachment #8450902 -
Flags: review?(21) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 9•10 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•10 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•10 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)
Updated•10 years ago
|
Target Milestone: --- → 2.0 S6 (18july)
Updated•10 years ago
|
Whiteboard: [c=regression p= s= u=2.0] [systemsfe] → [c=regression p= s= u=2.0] [systemsfe][MemShrink]
Updated•10 years ago
|
Whiteboard: [c=regression p= s= u=2.0] [systemsfe][MemShrink] → [c=regression p= s= u=2.0] [systemsfe][MemShrink:P1]
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Comment 13•10 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)
Comment 14•10 years ago
|
||
When comparing user experiences, make sure we're testing with 256mb Flame (273mb configuration.)
Reporter | ||
Comment 15•10 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•10 years ago
|
||
bug 1027231 has landed, make sure to test with that patch when looking at a layer tree.
Assignee | ||
Comment 17•10 years ago
|
||
Thanks for the pointers, I'll make a new patch today and test it with a 273MB flame.
Assignee | ||
Comment 18•10 years ago
|
||
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•10 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 20•10 years ago
|
||
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•10 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → fixed
Comment 23•10 years ago
|
||
Updated•10 years ago
|
Comment 24•10 years ago
|
||
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)
Updated•10 years ago
|
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.
Description
•