The lock screen is fairly simple: an absolutely positioned <div> with an image and some text in it. It's moved around on screen with CSS translations in response to touchmove events. The lock screen was horrifically slow initially but got better with ajuma's fixes for texture uploading on EGL. We still weren't able to repaint quickly enough to keep up with input events, so there was some lag. Recently we added a gradient (ohnoes!) to the lock screen and that slowed the framerate down. It's regressed around to the point it was before the texture-upload fix. This shouldn't be the case! In the meantime we've landed two optimizations that should apply perfectly to the lock screen - if an active transformed frame is the size of the screen or smaller, speculatively paint its entire computed size instead of just its visible region. * The lock screen is transformed and, well, the size of the screen. - don't reflow transformed frames on transform changes (--> don't invalidate) * The lock screen is only ever transformed. I'm going to poke around and see what's slow in here. This is one of the simplest possible cases to optimize. We should just be blitting a prerendered texture at the position specified by the CSS transform.

I should add, this is slow on the Galaxy S II, which has a high-end GPU and tons of memory bandwidth.

Confirmed that replacing the lock screen contents with a bare pink screen fixes perf problem.

While the frame is moving around, I see Painting --- before optimization (dirty 0,0,28800,48000): which is 480x800 pixels. We *are* hitting the prerender-partially-visible-frame optimization, but it's likely hurting us because we're invalidating the entire screen on each paint. Will try to see how far I can get in figuring out what's overinvalidating.

Another small bit of relevant information WIDGET: dirty <x=0,y=207,w=480,h=593> Painting --- before optimization (dirty 0,0,28800,48000): The visible area of the "lock screen", as it moves over the "home screen", is being dirtied in nsWindow. This is unexpected after bug 524925 (to me!) because layout of the transformed frame can't change. By the time get to nsLayoutUtils::PaintFrame, we've picked up the rest of the screen area to invalidate. That might be because there's a <canvas> under the lock screen. Poking some more.

With a plain-pink HTML background, instead of the canvas, same category of result WIDGET: dirty <x=0,y=111,w=480,h=689> Painting --- before optimization (dirty 0,0,28800,48000): and still slow.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #3) > Painting --- before optimization (dirty 0,0,28800,48000): That's not the area that's invalid in the widget. It's the area we're building display lists for, which is the whole window. That should be renamed...

What's happening is that we get to void nsIFrame::InvalidateTransformLayer() { NS_ASSERTION(mParent, "How can a viewport frame have a transform?"); bool hasLayer = FrameLayerBuilder::GetDedicatedLayer(this, nsDisplayItem::TYPE_TRANSFORM) != nsnull; but |hasLayer| is false o_O. The layer tree begs to differ. Poking some more.

So, - bool hasLayer = - FrameLayerBuilder::GetDedicatedLayer(this, nsDisplayItem::TYPE_TRANSFORM) != nsnull; + bool hasOrWillHaveLayer = + AreLayersMarkedActive(nsChangeHint_UpdateTransformLayer); results in the behavior I expect: one paint when the frame goes active and we layerize, and nothing thereafter. Still groping around in the dark somewhat.

Forgot to add, the speculative patch in comment 8 makes the on-device lockscreen fast enough to keep up with input events.

While the transition is active, the text within the transformed frame is invalidated and repainted. Commenting out the rule marked /* Comment out this rule, and the extra repaints stop. */ makes the extra repaints stop, as you might expect.

More info for my records. BAD display list (unexpected invalidation) REPAINTING THEBES <x=0,y=0,w=73,h=19> Painting --- after optimization: Clip 0x4364cb58(FrameOuter(browser)(4)) (0,0,28800,48000)(0,0,28800,48000) opaque OwnLayer 0x43f2e808(Viewport(-1)) (0,0,28800,48000)(0,0,28800,48000) opaque layer=0x40c3dda0 Clip 0x43f29408(HTMLScroll(div)(1)) (0,0,28800,48000)(0,0,28800,48000) opaque Background 0x43f43e58(HTMLScroll(body)(2)) (0,0,28800,48000)(0,0,28800,39312) opaque uniform layer=0x436a5c00 nsDisplayTransform 0x43f29408(HTMLScroll(div)(1)) (0,39312,28800,48001)(0,39312,28800,8688) opaque layer=0x40c3dae0 Background 0x43f29408(HTMLScroll(div)(1)) (0,0,28800,48000)(0,0,4380,1140) opaque uniform layer=0x4393b800 Clip 0x43f29810(Block(div)(1)) (0,0,28800,48000)(0,0,4339,1140) Text 0x43f8d7d0(Text(0)"Hello kitty") (0,0,4339,1140)(0,0,4339,1140) layer=0x4393b800 GOOD display list (no invalidation) Painting --- after optimization: Clip 0x4364cb58(FrameOuter(browser)(4)) (0,0,28800,48000)(0,0,28800,48000) opaque OwnLayer 0x43821808(Viewport(-1)) (0,0,28800,48000)(0,0,28800,48000) opaque layer=0x40c3dda0 Clip 0x43f41408(HTMLScroll(div)(1)) (0,0,28800,48000)(0,0,28800,48000) opaque Background 0x43f2ce58(HTMLScroll(body)(2)) (0,0,28800,48000)(0,0,28800,48000) opaque uniform layer=0x436a5800 nsDisplayTransform 0x43f41408(HTMLScroll(div)(1)) (0,20927,28800,48001)(0,20927,28800,27073) layer=0x40c3dae0 Background 0x43f41408(HTMLScroll(div)(1)) (0,0,28800,48000)(0,0,28800,48000) opaque uniform layer=0x4393b800 Clip 0x0() (0,0,28800,48000)(0,0,4339,1140) Text 0x43f8d6e0(Text(0)"Hello kitty") (0,0,4339,1140)(0,0,4339,1140) layer=0x4393b800 The only major difference is "Clip 0x43f29810(Block(div)(1))" (bad) vs. "Clip 0x0()" (good).

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12) > Created attachment 593336 [details] [diff] [review] > part 1: fix Makes the slow go away on device. Thanks!

Comment on attachment 593336 [details] [diff] [review] part 1: fix Tragically this patch isn't right. A frame F with 'opacity' can have a placeholder descendant whose out-of-flow frame is not a descendant of F. In that situation, this patch will cause us to not update the out-of-flow frame correctly. I will write a test for this. A similar situation might apply with a transformed inline that contains a float placeholder. Need to test since I have no idea how that works currently :-). What we really need here is a way to quickly identify the situation where a frame F has no placeholder descendants whose out-of-flows are outside F. That's the only situation where UpdateViewsForTree is needed when nsChangeHint_SyncFrameView is not present. Currently, when we have only nsChangeHint_RepaintFrame, it walks the entire frame subtree checking for this one rare situation.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18) > Comment on attachment 593336 [details] [diff] [review] > part 1: fix > > Tragically this patch isn't right. > > A frame F with 'opacity' can have a placeholder descendant whose out-of-flow > frame is not a descendant of F. In that situation, this patch will cause us > to not update the out-of-flow frame correctly. I will write a test for this. > > A similar situation might apply with a transformed inline that contains a > float placeholder. Need to test since I have no idea how that works > currently :-). Transforms are only supposed to apply to block-level or atomic-inline elements, which means they must be the containing blocks for any float descendants. And they're defined to be the containing block for any positioned descendants too, so transforms are fine here. For now, I'll fix the patch to only handle transforms and leave opacity with sometimes-unnecessary invalidation.

Hi Mats, do you have an ETA on reviews here? We critically need these fixes, see bug 724189 comment 16.

Comment on attachment 593625 [details] [diff] [review] Part 1 v2 > layout/base/nsCSSFrameConstructor.cpp >+ // We don't need to update transforms in UpdateViewsForTree, because >+ // there can't be any out-of-flows or popups that need to be transformed; >+ // all out-of-flow descendants of the transformed element must also be >+ // descendants of the transformed frame. IOW, an element with a transform makes it an abs.pos. container. Could we assert that the assumption holds first in ApplyRenderingChangeToTree() ? Something like: NS_ASSERTION(!(aChange & nsChangeHint_UpdateTransformLayer) || aFrame->GetStyleDisplay()->HasTransform(), ""); r=mats

Comment on attachment 593363 [details] [diff] [review] Part 5: more cleanup >+ // skip out of flows, they'll be handled as we traverse through placeholders nit: s/out of flows/out-of-flows/ and make it less than 80 columns >- if ((child->GetStateBits() & NS_FRAME_HAS_CONTAINER_LAYER) && >- (aChange & nsChangeHint_RepaintFrame)) { >- FrameLayerBuilder::InvalidateThebesLayerContents(child, >- child->GetVisualOverflowRectRelativeToSelf()); >- } >... >@@ -7724,16 +7724,17 @@ DoApplyRenderingChangeToTree(nsIFrame* a >... >+ FrameLayerBuilder::InvalidateThebesLayersInSubtree(aFrame); SyncViewsAndInvalidateDescendants calls DoApplyRenderingChangeToTree for each descendant frame, but InvalidateThebesLayersInSubtree also recurse on frame descendants in InternalInvalidateThebesLayersInSubtree so this seems sub-optimal.

(In reply to Mats Palmgren [:mats] from comment #24) > SyncViewsAndInvalidateDescendants calls DoApplyRenderingChangeToTree for > each descendant frame, Only for descendant out-of-flows and popups. > but InvalidateThebesLayersInSubtree also recurse > on frame descendants in InternalInvalidateThebesLayersInSubtree so this > seems sub-optimal. Only for descendants that have their own ContainerLayers. SyncViewsAndInvalidateDescendants only needs to call DoApplyRenderingChangeToTree for out-of-flows that aren't descendants of the original frame. Maybe I should add a check for that?

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed06563619f6 https://hg.mozilla.org/integration/mozilla-inbound/rev/6784eefe33a6 Please leave open after inbound landing, since more patches need to land.

> Only for descendant out-of-flows and popups. Yes, we trampoline off the placeholder and skip all the out-of-flows so we won't do them twice, but DoApplyRenderingChangeToTree calls SyncViewsAndInvalidateDescendants(aFrame), so for this frame tree: top placeholder-1 => abs-pos-1 abs-pos-1 placeholder-2 => abs-pos-2 abs-pos-2 we do these calls: DoApplyRenderingChangeToTree(abs-pos-1) SyncViewsAndInvalidateDescendants(abs-pos-1) DoApplyRenderingChangeToTree(abs-pos-2) SyncViewsAndInvalidateDescendants(abs-pos-2) InvalidateThebesLayersInSubtree(abs-pos-2) InvalidateThebesLayersInSubtree(abs-pos-1) InvalidateThebesLayersInSubtree recurse all descendants unconditionally: http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#1951 so InvalidateThebesLayersInSubtree(abs-pos-1) will process abs-pos-2. > SyncViewsAndInvalidateDescendants only needs to call > DoApplyRenderingChangeToTree for out-of-flows that aren't descendants of > the original frame. Hmm, how do we process UpdateOpacityLayer for abs-pos-1 if we skip calling DoApplyRenderingChangeToTree for it?

https://hg.mozilla.org/mozilla-central/rev/ed06563619f6 https://hg.mozilla.org/mozilla-central/rev/6784eefe33a6

Comment on attachment 593348 [details] [diff] [review] Part 2: test The test file is chock full of \r.

Comment on attachment 593363 [details] [diff] [review] Part 5: more cleanup r- until comment 27 is addressed (to avoid Bugzilla review reminders)

Let's not do anything more here until DLBI has landed.

The test invalidates well for me, but I don't remember all the work we starting doing here. Is there anything we want to keep post-DLBI?

Looks like there's a test from bug 724189 (attachment 594634 [details] [diff] [review]) that we wanted to land along with this.

I'll land parts 3 and 4.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a1e0e44245 https://hg.mozilla.org/integration/mozilla-inbound/rev/329f3f03ee1c

https://hg.mozilla.org/mozilla-central/rev/d3a1e0e44245 https://hg.mozilla.org/mozilla-central/rev/329f3f03ee1c