We could kill ~25% of the samples in bug 780341 if we bypassed the display-list + layerbuilder machinery for the (extremely simple) transform-only changes triggered by the test. The conditions in which we could take this fast path are, the frame is - transformed - prerendered (which implies active) - *not* opaque The difficulty in implementing this, as opposed to empty transactions for canvas/video, is that we need to actually update the transform on the prerendered frame's layer. It doesn't look too hard to compute the transform without a display list [1]. However, what *would* be hard outside of a display list is finding a rendering resolution, in full generality. I think we could cut a corner here by only attempting this optimization when there's no scaling applied to the frame. Would you guys be comfortable with that special of an optimization? If so, I think it's worth attempting because of the huge potential win for this very common case (translation-only transforms). [1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#3418 [2] http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#2164

I had hoped this (terrible) patch would not break rendering enough for me to see what the effect of this optimization might be, but unfortunately we never attempt an empty transaction, even though I confirmed that InvalidateTransformLayer() is requesting one. It appears that something else is invalidating here.

Here's where the invalidations seem to be coming from #0 nsIFrame::InvalidateRoot (this=0x4593b7f8, aDamageRect=..., aFlags=16) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4948 #1 0x407f9a16 in ViewportFrame::InvalidateInternal (this=0x4593b7f8, aDamageRect=..., aX=9600, aY=180, aForChild=0x45ca9880, aFlags=16) at /home/cjones/mozilla/inbound/layout/generic/nsViewportFrame.cpp:317 #2 0x4077b9b6 in nsIFrame::InvalidateInternalAfterResize (this=0x45ca9880, aDamageRect=..., aX=9600, aY=180, aFlags=16) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4808 #3 0x4077ba68 in nsIFrame::InvalidateInternal (this=0x45ca9880, aDamageRect=..., aX=9600, aY=180, aForChild=0x45ca9880, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4828 #4 0x4074dcd0 in nsBlockFrame::InvalidateInternal (this=0x45ca9880, aDamageRect=..., aX=9600, aY=180, aForChild=0x46c6aef0, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsBlockFrame.cpp:513 #5 0x4077b97a in nsIFrame::InvalidateInternalAfterResize (this=0x46c6aef0, aDamageRect=..., aX=9600, aY=180, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4804 #6 0x4077ba68 in nsIFrame::InvalidateInternal (this=0x46c6aef0, aDamageRect=..., aX=9600, aY=180, aForChild=0x46c6aef0, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4828 #7 0x4074dcd0 in nsBlockFrame::InvalidateInternal (this=0x46c6aef0, aDamageRect=..., aX=9600, aY=180, aForChild=0x46c6af88, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsBlockFrame.cpp:513 #8 0x4077b9b6 in nsIFrame::InvalidateInternalAfterResize (this=0x46c6af88, aDamageRect=..., aX=9600, aY=0, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4808 #9 0x4077ba68 in nsIFrame::InvalidateInternal (this=0x46c6af88, aDamageRect=..., aX=9600, aY=0, aForChild=0x46c6af88, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4828 #10 0x4074dcd0 in nsBlockFrame::InvalidateInternal (this=0x46c6af88, aDamageRect=..., aX=9600, aY=0, aForChild=0x46c6b7a8, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsBlockFrame.cpp:513 #11 0x4077b9b6 in nsIFrame::InvalidateInternalAfterResize (this=0x46c6b7a8, aDamageRect=..., aX=0, aY=0, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4808 #12 0x4077ba68 in nsIFrame::InvalidateInternal (this=0x46c6b7a8, aDamageRect=..., aX=0, aY=0, aForChild=0x46c6b7a8, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4828 #13 0x4074dcd0 in nsBlockFrame::InvalidateInternal (this=0x46c6b7a8, aDamageRect=..., aX=0, aY=0, aForChild=0x0, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsBlockFrame.cpp:513 #14 0x4077b7c8 in nsIFrame::InvalidateWithFlags (this=0x46c6b7a8, aDamageRect=..., aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4721 #15 0x406b9626 in nsIFrame::Invalidate (this=0x46c6b7a8, aDamageRect=...) at /home/cjones/mozilla/inbound/layout/base/../generic/nsIFrame.h:2159 #16 0x4077be30 in nsIFrame::InvalidateOverflowRect (this=0x46c6b7a8) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4917 #17 0x40698b68 in DoApplyRenderingChangeToTree (aFrame=0x46c6b7a8, aFrameManager=0x43622600, aChange=33) at /home/cjones/mozilla/inbound/layout/base/nsCSSFrameConstructor.cpp:7745 #18 0x4069895e in UpdateViewsForTree (aFrame=0x46c6af88, aFrameManager=0x43622600, aChange=33) at /home/cjones/mozilla/inbound/layout/base/nsCSSFrameConstructor.cpp:7693 #19 0x40698a22 in UpdateViewsForTree (aFrame=0x46c6aef0, aFrameManager=0x43622600, aChange=33) at /home/cjones/mozilla/inbound/layout/base/nsCSSFrameConstructor.cpp:7704 #20 0x40698ae8 in DoApplyRenderingChangeToTree (aFrame=0x46c6aef0, aFrameManager=0x43622600, aChange=2593) at /home/cjones/mozilla/inbound/layout/base/nsCSSFrameConstructor.cpp:7731 #21 0x40698d96 in ApplyRenderingChangeToTree (aPresContext=0x4529a800, aFrame=0x46c6aef0, aChange=2593) at /home/cjones/mozilla/inbound/layout/base/nsCSSFrameConstructor.cpp:7817 #22 0x406995f0 in nsCSSFrameConstructor::ProcessRestyledFrames (this=0x43622600, aChangeList=...) at /home/cjones/mozilla/inbound/layout/base/nsCSSFrameConstructor.cpp:8104 #23 0x40699b5e in nsCSSFrameConstructor::RestyleElement (this=0x43622600, aElement=0x450bc6a0, aPrimaryFrame=0x45ca87f8, aMinHint=0, aRestyleTracker=..., aRestyleDescendants=true) at /home/cjones/mozilla/inbound/layout/base/nsCSSFrameConstructor.cpp:8244 #24 0x406846c4 in mozilla::css::RestyleTracker::ProcessOneRestyle (this=0x43622688, aElement=0x450bc6a0, aRestyleHint=eRestyle_Subtree, aChangeHint=0) at /home/cjones/mozilla/inbound/layout/base/RestyleTracker.cpp:124 #25 0x40684b82 in mozilla::css::RestyleTracker::DoProcessRestyles (this=0x43622688) at /home/cjones/mozilla/inbound/layout/base/RestyleTracker.cpp:209 #26 0x40688a82 in mozilla::css::RestyleTracker::ProcessRestyles (this=0x43622688) at /home/cjones/mozilla/inbound/layout/base/RestyleTracker.h:68 #27 0x406a04d8 in nsCSSFrameConstructor::ProcessPendingRestyles (this=0x43622600) at /home/cjones/mozilla/inbound/layout/base/nsCSSFrameConstructor.cpp:12066 #28 0x4070d316 in PresShell::FlushPendingNotifications (this=0x445582c0, aType=Flush_Layout) at /home/cjones/mozilla/inbound/layout/base/nsPresShell.cpp:3854 #29 0x40712248 in PresShell::HandleEvent (this=0x445582c0, aFrame=0x4593b7f8, aEvent=0xbecc1fc0, aDontRetargetEvents=false, aEventStatus=0xbecc1e7c) at /home/cjones/mozilla/inbound/layout/base/nsPresShell.cpp:5778 The change hint is 33, which appears to be nsChangeHint_RepaintFrame | nsChangeHint_SyncFrameView.

With http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5760 hacked out, we seem to be canceling the empty transaction through Breakpoint 1, nsIFrame::InvalidateRoot (this=0x4583b7f8, aDamageRect=..., aFlags=16) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4948 #0 nsIFrame::InvalidateRoot (this=0x4583b7f8, aDamageRect=..., aFlags=16) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4948 #1 0x408189de in ViewportFrame::InvalidateInternal (this=0x4583b7f8, aDamageRect=..., aX=0, aY=0, aForChild=0x45ba9880, aFlags=16) at /home/cjones/mozilla/inbound/layout/generic/nsViewportFrame.cpp:317 #2 0x4079a97e in nsIFrame::InvalidateInternalAfterResize (this=0x45ba9880, aDamageRect=..., aX=0, aY=0, aFlags=16) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4808 #3 0x4079aa30 in nsIFrame::InvalidateInternal (this=0x45ba9880, aDamageRect=..., aX=0, aY=0, aForChild=0x45ba9880, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4828 #4 0x4076cc98 in nsBlockFrame::InvalidateInternal (this=0x45ba9880, aDamageRect=..., aX=0, aY=0, aForChild=0x458ade78, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsBlockFrame.cpp:513 #5 0x4079a942 in nsIFrame::InvalidateInternalAfterResize (this=0x458ade78, aDamageRect=..., aX=0, aY=0, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4804 #6 0x4079aa30 in nsIFrame::InvalidateInternal (this=0x458ade78, aDamageRect=..., aX=0, aY=0, aForChild=0x458ade78, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4828 #7 0x4076cc98 in nsBlockFrame::InvalidateInternal (this=0x458ade78, aDamageRect=..., aX=0, aY=0, aForChild=0x0, aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsBlockFrame.cpp:513 #8 0x4079a790 in nsIFrame::InvalidateWithFlags (this=0x458ade78, aDamageRect=..., aFlags=0) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4721 #9 0x406d8626 in nsIFrame::Invalidate (this=0x458ade78, aDamageRect=...) at /home/cjones/mozilla/inbound/layout/base/../generic/nsIFrame.h:2159 #10 0x4079a2ca in nsIFrame::InvalidateLayer (this=0x458ade78, aDamageRect=..., aDisplayItemKey=58) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:4564 #11 0x407a07e8 in nsIFrame::FinishAndStoreOverflow (this=0x458ade78, aOverflowAreas=..., aNewSize=...) at /home/cjones/mozilla/inbound/layout/generic/nsFrame.cpp:7074 #12 0x406b8818 in nsCSSFrameConstructor::ProcessRestyledFrames (this=0x43622600, aChangeList=...) at /home/cjones/mozilla/inbound/layout/base/nsCSSFrameConstructor.cpp:8155 #13 0x406b8b5e in nsCSSFrameConstructor::RestyleElement (this=0x43622600, aElement=0x45cc73d0, aPrimaryFrame=0x458ade78, aMinHint=0, aRestyleTracker=..., aRestyleDescendants=false) at /home/cjones/mozilla/inbound/layout/base/nsCSSFrameConstructor.cpp:8244 #14 0x406a36c4 in mozilla::css::RestyleTracker::ProcessOneRestyle (this=0x43622688, aElement=0x45cc73d0, aRestyleHint=eRestyle_Self, aChangeHint=0) at /home/cjones/mozilla/inbound/layout/base/RestyleTracker.cpp:124 #15 0x406a3b82 in mozilla::css::RestyleTracker::DoProcessRestyles (this=0x43622688) at /home/cjones/mozilla/inbound/layout/base/RestyleTracker.cpp:209 #16 0x406a7a82 in mozilla::css::RestyleTracker::ProcessRestyles (this=0x43622688) at /home/cjones/mozilla/inbound/layout/base/RestyleTracker.h:68 #17 0x406bf4d8 in nsCSSFrameConstructor::ProcessPendingRestyles (this=0x43622600) at /home/cjones/mozilla/inbound/layout/base/nsCSSFrameConstructor.cpp:12066 #18 0x4072c316 in PresShell::FlushPendingNotifications (this=0x444582c0, aType=Flush_Style) at /home/cjones/mozilla/inbound/layout/base/nsPresShell.cpp:3854 #19 0x4073de7a in nsRefreshDriver::Notify (this=0x444581a0, aTimer=0x4542ca10) at /home/cjones/mozilla/inbound/layout/base/nsRefreshDriver.cpp:378

What's happening is that the empty txn is being cancelled because an nsIFrame for an nsDisplayTransform (key = 58) above doesn't have its own layer, so goes through the normal Invalidate() path. That's weird, because it means we're InvalidateLayer()ing frames that aren't visible, and that's affecting the layers txn for frames that are visible. If this kind of invalidate happens often, it also means this optimization will be exceptionally fragile ...

This p.o.c. gives an idea of what we can win from this optimization. It's 15-16 fps! (~37->53) Will grab a profile to confirm what should be happening, but my logging suggests what we expect to happen is, bypassing display-list + layerbuilder overhead. But, several problems arose. 1. dz landed a patch to flush style just for touch events in PresShell, which I don't understand. Whatever it's doing cancels the empty transaction. 2. We nsIFrame::InvalidateLayer()/InvalidateTransformLayer() for transformed layers that are not visible (comment 4), and because they don't have a dedicated layer this invalidates the root and cancels the empty transaction. It makes sense to me why we do this, but this also would prevent the optimization from applying. We need a plan for avoiding these. 3. Perhaps worst of all, even if we fixed (1) and (2), and made this PoC patch actually update the transform, we still end up with a shadow layers transaction with no edits. This results in the Update being skipped. We somehow need to update the transform on the ContainerLayer in such a way that it gets an OpEdit in the layers txn.

As expected, this essentially drops the expensive parts of PresShell::Paint (sans IPC) out of the profile http://people.mozilla.com/~bgirard/cleopatra/?report=f64f2bc53fb607158d36259a43f3b91c5315d8a9

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5) > Created attachment 660379 [details] [diff] [review] > Proof of concept (dirty hack) > > 1. dz landed a patch to flush style just for touch events in PresShell, > which I don't understand. Whatever it's doing cancels the empty transaction. > Will do some mxrchaeology to see why this was added. > 2. We nsIFrame::InvalidateLayer()/InvalidateTransformLayer() for > transformed layers that are not visible (comment 4), and because they don't > have a dedicated layer this invalidates the root and cancels the empty > transaction. It makes sense to me why we do this, but this also would > prevent the optimization from applying. We need a plan for avoiding these. > Matt notes that DLBI will give us the capability to check if a frame had a display item in the last paint, which is *exactly* what we want to do to skip the full invalidation here. Vastly preferable to the hackery I had in mind. > 3. Perhaps worst of all, even if we fixed (1) and (2), and made this PoC > patch actually update the transform, we still end up with a shadow layers > transaction with no edits. This results in the Update being skipped. We > somehow need to update the transform on the ContainerLayer in such a way > that it gets an OpEdit in the layers txn. This doesn't seem so bad anymore; we grab the layer, set the transform, and set a bit that makes it add OpSetLayerAttributes during the shadow layers txn. So the last question is really > I think we could cut a corner here by only attempting this optimization when there's no > scaling applied to the frame. Would you guys be comfortable with that special of an > optimization? If so, I think it's worth attempting because of the huge potential win for > this very common case (translation-only transforms).

dz, part 2 of bug 780692 is causing us to flush layout even when there aren't touch listeners, which is causing an invalidate that cancels empty layers transactions. Is there any way we can do something better?

Yes, we should only flush when there are touch listeners. I didn't add a check for that because I thought we would always have touch listeners, but I guess that's not true.

Well, we have mouse listeners, and dispatching mouse events doesn't seem to flush layout (at least, it doesn't cancel the empty transaction). Why the special case for touch events?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #5) > > 2. We nsIFrame::InvalidateLayer()/InvalidateTransformLayer() for > > transformed layers that are not visible (comment 4), and because they don't > > have a dedicated layer this invalidates the root and cancels the empty > > transaction. It makes sense to me why we do this, but this also would > > prevent the optimization from applying. We need a plan for avoiding these. > > > > Matt notes that DLBI will give us the capability to check if a frame had a > display item in the last paint, which is *exactly* what we want to do to > skip the full invalidation here. Vastly preferable to the hackery I had in > mind. > I had to work through this logic again. Will write it down this time. Here are all the cases == prerendered -> (still visible, so still prerendered) == 1. transformed frame A was prerendered during paint i 2. A's transform is changed and it's now still visible. We should have everything we need to compute that. 3. we invalidate for A 4. any other frame-tree state change happens We're free to request an empty transaction if A's layer is not opaque content. If it's opaque, the displaylist may have optimized away items covered by A that would need to painted in i+1. If something in (4) moves A hidden, then it must cancel the empty txn, and we'll de-layerize A (freeing its surface). == hidden -> hidden == 1. transformed frame A was hidden during paint i. DLBI provides the tool we need to compute that. 2. A's transform is changed and it's now still hidden 3. we invalidate for A 4. any other frame-tree state change happens We're free to request an empty transaction. If any state change occurs in (4) to make A visible, then the invalidation for that state change must cancel the empty txn. These two cases are what we see on the gaia homescreen. == prerendered -> hidden == We can choose whether or not to request an empty txn. AFAICT it's correct to request an empty txn, but if we do so then I think it's possible for A's buffer to stay alive indefinitely (until the next painting txn). So we can choose memory/speed here. (This probably happens a fair bit on the gaia homescreen, so we have a good testcase already.) == hidden -> visible == We can never request an empty txn. == visible -> { visible, hidden } == It may be possible in theory to check if we can request an empty txn for visible->visible, but doesn't seem worth it. Skip.

Note to self: displayport changes will cancel an empty txn.

This eliminates the removal of the Flush(Layout) on touch events, since that doesn't cancel the txns when I retest. So that's one problem down.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #11) > We're free to request an empty transaction if A's layer is not opaque > content. If it's opaque, the displaylist may have optimized away items > covered by A that would need to painted in i+1. That's not quite true. In general display-list processing might have removed stuff covered by some opaque content that didn't end up covering the entire layer, so the layer was not marked opaque. Maybe instead nsDisplayTransform::GetOpaqueRegion should just return empty always. Then you wouldn't have to worry about this at all. Off the top of my head I don't know of any situations where it's important. The rest sounds right.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #11) > > We're free to request an empty transaction if A's layer is not opaque > > content. If it's opaque, the displaylist may have optimized away items > > covered by A that would need to painted in i+1. > > That's not quite true. In general display-list processing might have removed > stuff covered by some opaque content that didn't end up covering the entire > layer, so the layer was not marked opaque. > Right-o, I see. > Maybe instead nsDisplayTransform::GetOpaqueRegion should just return empty > always. Then you wouldn't have to worry about this at all. Off the top of my > head I don't know of any situations where it's important. > With prerendered layers moving over other content, this might even improve things, right? We wouldn't have to invalidate the underneath content as it moves in/out of opaque regions of the prerender frame. > The rest sounds right. Whew! Thanks for looking over this.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #15) > With prerendered layers moving over other content, this might even improve > things, right? We wouldn't have to invalidate the underneath content as it > moves in/out of opaque regions of the prerender frame. Yes.

On vanilla 5245844b645d, the gaia homescreen pans at a stable 35-36fps.

What remains here is a part 3 that - ensures that invalidations of transformed frames only schedule an empty txn - sets the updated transform on the frame's Layer* using the API in part 2

Matt pointed me at DoApplyRenderingChangeToTree() (nsCSSFrameConstructor.cpp). From here, there are three things we need to run the algorithm in comment 11 1. Is the frame prerendered (ShouldPrerenderTransformedContent()). Currently, we need a display list builder just to find a root reference frame. 2. Did the frame have a display item in the last paint 3. Is (or might) the frame currently be visible. False positives ("might be visible" when it's not) are OK, but false negatives ("isn't visible" when it is) will kill the optimization.

The new plan is to - set a layer user-data key when we prerender a transform layer - add a nsIFrame::TryUpdateTransformOnly() method. Return true if we can get away with only updating the frame's layer's transform (comment 11). - in DoApplyRenderingChangeToTree(), run SchedulePaint(COMPOSITE_ONLY) if nothing asks for needInvalidatingRepaint.

This patch makes for fast when I pan between a page with icons and one that's empty. However, in the interesting case (panning between two pages with icons), we still end up SchedulePaint(Invalidating). It's not DoApplyRenderingChangeToTree() that's asking for the invalidating paint (I added logging there), so I'll try to find out what is.

Comment on attachment 666421 [details] [diff] [review] PoC of Part 3, v3 Review of attachment 666421 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSFrameConstructor.cpp @@ +7752,5 @@ > } > } else { > + // XXX/review do we need to force an invalidating repaint or > + // will the recursion do the right thing? > + needInvalidatingPaint = true; Anything that calls InvalidateFrame() will schedule a paint. These 3 changes could also be put once, below the next } ::: layout/generic/nsIFrame.h @@ +2276,1 @@ > PAINT_COMPOSITE_ONLY Hrm, this is really bad. I feel like empty transactions probably don't work at all currently. I wonder which one my various rebases broke that.

(In reply to Matt Woodrow (:mattwoodrow) from comment #24) > Comment on attachment 666421 [details] [diff] [review] > PoC of Part 3, v3 > > Review of attachment 666421 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/base/nsCSSFrameConstructor.cpp > @@ +7752,5 @@ > > } > > } else { > > + // XXX/review do we need to force an invalidating repaint or > > + // will the recursion do the right thing? > > + needInvalidatingPaint = true; > > Anything that calls InvalidateFrame() will schedule a paint. These 3 changes > could also be put once, below the next } > I can do that, but I wrote this to allow the different basic blocks to schedule paints differently.

It's UpdateViewsForTree() that's killing things: http://pastebin.mozilla.org/1852468 Not sure why yet.

We're invalidating because nsChangeHint_UpdateOpacityLayer is getting passed in.

So I think I need to fix my FIXME/bug XXXXX comment in PoC v3.

Adds opacity.

This is less a "how should I do this f?", and more of a "is this totally broken f?". I think it's still right based on the argument in comment 11, but would like another pair of eyes on it.

Comment on attachment 666452 [details] [diff] [review] PoC of Part 3, v4 Review of attachment 666452 [details] [diff] [review]: ----------------------------------------------------------------- Looks good in theory :) I think the hard part will be the 'is prerendered' tag for the nsDisplayOpacity. We only choose to prerendered nsDisplayTransform's, and the nsDisplayOpacity will be a child of this. I guess the 'is prerendered' check should be if the current layer, or any ancestor layer has the user data attached.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #11) > == hidden -> hidden == > > 1. transformed frame A was hidden during paint i. DLBI provides the tool > we need to compute that. I think this case may be canceling the empty transactions. Now that opacity is in the picture because of gaia changes (it wasn't before), this might be a bit more complicated. Will think on't.

I partially fixed the hidden->hidden case from comment 11, (modulo bool mightBeVisibleCurrently = false; // TODO which I need some help to compute), but I'm still seeing a frame that has a dedicated layer canceling the empty transaction. I suspect this is related to the new opacity wrinkle that arose with gaia changes, which seems totally unnecessary in the new UX. Pinged vingtetun to follow up.

I should note that what I see suggests opacity layers as parents of transformed layers, which seems to be unexpected.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #33) > I partially fixed the hidden->hidden case from comment 11, (modulo > > bool mightBeVisibleCurrently = false; // TODO > This seems not to be necessary anymore, but I'm not sure why. Pre-DLBI, I found that transformed frames that weren't visible would cancel empty transactions, but they don't seem to be doing that anymore post-dlbi.

(In reply to Matt Woodrow (:mattwoodrow) from comment #31) > Comment on attachment 666452 [details] [diff] [review] > PoC of Part 3, v4 > > Review of attachment 666452 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good in theory :) > > I think the hard part will be the 'is prerendered' tag for the > nsDisplayOpacity. > > We only choose to prerendered nsDisplayTransform's, and the nsDisplayOpacity > will be a child of this. > I decided to punt opacity. There's an impending gaia change that will make optimizing this case for opacity less critical.

Ended up being far less painful that I thought it might end up being.

Comment on attachment 666801 [details] [diff] [review] part 3: If we're just updating the transform of a prerendered layer, then schedule an empty transaction to skip unnecessary display-list overhead Review of attachment 666801 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFrame.cpp @@ +4857,5 @@ > + gfxMatrix transform; > + // FIXME/bug XXXXXX: we currently bail on non-translation matrices, > + // because we sometimes render at different resolutions depending on > + // the scale factor and we don't have enough context to deduce the > + // right resolution here. Could we do slightly better and only bail if the difference between the matrices isn't only a translation? ::: layout/generic/nsIFrame.h @@ +1275,5 @@ > + * Return true if this frame might be using a transform getter. > + * > + * XXX/review: nsPageFrame uses transform getters, but sets them on > + * child nsIFrames, not on itself, so this approach won't work for > + * that. Does anyone care? I don't think this matters. nsPageFrame should never be getting UpdateTransformLayer style change hints anyway, since it won't be 'transformed' from the style codes perspective.

Comment on attachment 666800 [details] [diff] [review] part 2: Add API for mutating shadowable-layer base transforms outside a layers transaction Review of attachment 666800 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/basic/BasicLayerManager.cpp @@ +872,5 @@ > // the container variable > BasicContainerLayer* container = static_cast<BasicContainerLayer*>(aLayer); > + if (container) { > + // XXX/review: is this the right place to put this? > + container->CheckMutatedOutsideTransaction(); I think every layer will go through this point, so you can just call this on aLayer (or container, but it's potentially an 'undefined' cast in lots of cases) and remove the other callers.

Oh my, I'm leaking nsDisplayTransform like a sieve on try. Will see what I need to do to release those guys.

Much better!

Adds an early check if we're already going to do an invalidating paint since the code here isn't free, expands the "can just set transform" check to cases where only the translation has *changed* (nice idea Matt!), and fixes the leak.

Removes xxx/review comment that Matt answered, and fills in my FIXME/bug XXXXXX comments.

Comment on attachment 666857 [details] [diff] [review] part 3: If we're just updating the transform of a prerendered layer, then schedule an empty transaction to skip unnecessary display-list overhead, v2 Review of attachment 666857 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSFrameConstructor.cpp @@ +7770,5 @@ > + // layer for this frame, and not scheduling an invalidating > + // paint. > + if (!needInvalidatingPaint && > + !nsLayoutUtils::GetDisplayRootFrame(aFrame)->HasAnyStateBits( > + NS_FRAME_UPDATE_LAYER_TREE)) { You could put this check inside TryUpdateTransformOnly, since already compute GetDisplayRootFrame there. As discussed, this is fairly cheap, so it probably doesn't matter.

That makes sense.

Move the already-have-invalidating-paint-scheduled check into TryUpdateTransformOnly().

Comment on attachment 666856 [details] [diff] [review] part 2: Add API for mutating shadowable-layer base transforms outside a layers transaction, v2 Review of attachment 666856 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/Layers.h @@ +716,5 @@ > + * efficiently move prerendered layers around on screen that don't > + * invalidate other content when moved. > + * > + * The mutation here (if any) will be recorded in the next scheduled > + * transaction, empty or otherwise. This is kind of gross. It undermines the integrity of transactions. Could we queue somewhere a series of callbacks to be executed at the next empty transaction, or thrown away if the next transaction is not empty (or the layer manager is destroyed)? How about in FrameLayerBuilder's layer manager user data?

Comment on attachment 666869 [details] [diff] [review] part 3: If we're just updating the transform of a prerendered layer, then schedule an empty transaction to skip unnecessary display-list overhead, v3 Review of attachment 666869 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSFrameConstructor.cpp @@ +7751,5 @@ > nsSVGUtils::InvalidateBounds(aFrame); > } > } else { > + // XXX/review do we need to force an invalidating repaint or > + // will the recursion do the right thing? I think we should force an invalidating repaint. ::: layout/generic/nsFrame.cpp @@ +4860,5 @@ > + new (&builder) nsDisplayTransform(&builder, this, &list); > + > + transform3d = > + item->GetTransform(PresContext()->AppUnitsPerDevPixel()); > + list.DeleteAll(); Eep. Is this the only way? Is it really hard to avoid this and still share code? Can we at least hide this behind a static function on nsDisplayTransform? Or nsLayoutUtils? @@ +4866,5 @@ > + gfxMatrix transform, previousTransform; > + // FIXME/bug 796690 and 796705: if the matrix is 3d or something > + // other than the translation has changed, then we currently bail. > + // We don't have enough context to properly deal with changes other > + // than translations. I'm not sure what this refers to? ::: layout/generic/nsSimplePageSequence.h @@ +90,5 @@ > // We must allow Print Preview UI to have a background, no matter what the > // user's settings > virtual bool HonorPrintBackgroundSettings() { return false; } > > + virtual bool HasTransformGetter() const MOZ_OVERRIDE { return true; } I believe nsPageContentFrame needs this too

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #49) > Comment on attachment 666856 [details] [diff] [review] > part 2: Add API for mutating shadowable-layer base transforms outside a > layers transaction, v2 > > Review of attachment 666856 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/Layers.h > @@ +716,5 @@ > > + * efficiently move prerendered layers around on screen that don't > > + * invalidate other content when moved. > > + * > > + * The mutation here (if any) will be recorded in the next scheduled > > + * transaction, empty or otherwise. > > This is kind of gross. It undermines the integrity of transactions. > It's limited to a set of properties (currently just transform) that can be safely modified directly. > Could we queue somewhere a series of callbacks to be executed at the next > empty transaction, or thrown away if the next transaction is not empty (or > the layer manager is destroyed)? How about in FrameLayerBuilder's layer > manager user data? We could, but what do we gain from the significantly increased complexity? With callbacks, we'd have new problems with memory management, since the callbacks would have to hold strong or weak refs to the layers they wanted to modify. What problem are you trying to solve?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #50) > Comment on attachment 666869 [details] [diff] [review] > part 3: If we're just updating the transform of a prerendered layer, then > schedule an empty transaction to skip unnecessary display-list overhead, v3 > > ::: layout/generic/nsFrame.cpp > @@ +4860,5 @@ > > + new (&builder) nsDisplayTransform(&builder, this, &list); > > + > > + transform3d = > > + item->GetTransform(PresContext()->AppUnitsPerDevPixel()); > > + list.DeleteAll(); > > Eep. Is this the only way? Is it really hard to avoid this and still share > code? > The alternative is refactoring all the code that deals with transforms to live on nsIFrame, which is nontrivial and pretty likely to regress. (I'll get something wrong.) I'm not sure that's better. > Can we at least hide this behind a static function on nsDisplayTransform? Or > nsLayoutUtils? > Sure. > @@ +4866,5 @@ > > + gfxMatrix transform, previousTransform; > > + // FIXME/bug 796690 and 796705: if the matrix is 3d or something > > + // other than the translation has changed, then we currently bail. > > + // We don't have enough context to properly deal with changes other > > + // than translations. > > I'm not sure what this refers to? > If the transform's scale changes, e.g., we don't know whether we can schedule an empty transaction because we don't know if the rendering resolution would have changed because of the scale. > ::: layout/generic/nsSimplePageSequence.h > @@ +90,5 @@ > > // We must allow Print Preview UI to have a background, no matter what the > > // user's settings > > virtual bool HonorPrintBackgroundSettings() { return false; } > > > > + virtual bool HasTransformGetter() const MOZ_OVERRIDE { return true; } > > I believe nsPageContentFrame needs this too OK.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #51) > We could, but what do we gain from the significantly increased complexity? > With callbacks, we'd have new problems with memory management, since the > callbacks would have to hold strong or weak refs to the layers they wanted > to modify. What problem are you trying to solve? Clean semantics, which maybe boils down to just naming. "SetBaseTransformOutsideTransaction" suggests that we're just setting the transform willy-nilly and it's unclear when it will take effect. How about we call it "SetBaseTransformAtNextTransaction" and say that it takes effect at the beginning of the next transaction. Then the fact that you can implement it by immediately setting the transform stored in the layer is an implementation detail.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #52) > The alternative is refactoring all the code that deals with transforms to > live on nsIFrame, which is nontrivial and pretty likely to regress. (I'll > get something wrong.) I'm not sure that's better. I thought you could refactor nsDisplayTransform so the guts of it are static methods that take parameters (or possibly using a parameters object, or an internal object with state), passing in as parameters any values you need from nsDisplayListBuilder. > > Can we at least hide this behind a static function on nsDisplayTransform? Or > > nsLayoutUtils? > > Sure. Let's do that. > If the transform's scale changes, e.g., we don't know whether we can > schedule an empty transaction because we don't know if the rendering > resolution would have changed because of the scale. OK, add that as a comment and note that we could relax this. E.g. if we're only reducing the scale then we'd be OK to go ahead.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #53) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #51) > > We could, but what do we gain from the significantly increased complexity? > > With callbacks, we'd have new problems with memory management, since the > > callbacks would have to hold strong or weak refs to the layers they wanted > > to modify. What problem are you trying to solve? > > Clean semantics, which maybe boils down to just naming. > "SetBaseTransformOutsideTransaction" suggests that we're just setting the > transform willy-nilly and it's unclear when it will take effect. > OK, makes sense. > How about we call it "SetBaseTransformAtNextTransaction" and say that it > takes effect at the beginning of the next transaction. Then the fact that > you can implement it by immediately setting the transform stored in the > layer is an implementation detail. Maybe SetBaseTransformForNextTransaction()? Note that this isn't a transparent optimization, because clients will be able to GetBaseTransform() and see that it's taken effect.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #54) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #52) > > The alternative is refactoring all the code that deals with transforms to > > live on nsIFrame, which is nontrivial and pretty likely to regress. (I'll > > get something wrong.) I'm not sure that's better. > > I thought you could refactor nsDisplayTransform so the guts of it are static > methods that take parameters (or possibly using a parameters object, or an > internal object with state), passing in as parameters any values you need > from nsDisplayListBuilder. > Yes, this is possible. But when I started on this, I noticed that it was quite nontrivial. (Ab)using nsDisplayListBuilder to accomplish the same felt a little hackier, but it achieved the same purpose; the code is still shared. > > If the transform's scale changes, e.g., we don't know whether we can > > schedule an empty transaction because we don't know if the rendering > > resolution would have changed because of the scale. > > OK, add that as a comment and note that we could relax this. E.g. if we're > only reducing the scale then we'd be OK to go ahead. In general, we still might decide on a different resolution though, right? So it's more, "we could relax the resolution computation for downscales because that would be less bad in general", instead of "results would be the same whether or not we fast-pathed this transform update". (My proposal in bug 796705 was to just ignore the resolution computation for these fast-pathed transform updates. Lots of other solutions are possible)

carrying r=mattwoodrow

Comment on attachment 667292 [details] [diff] [review] part 3: Add a GetLayerTransformForFrame() helper for computing a frame's layer transform outside a proper transaction, and mark frames that have been fully prerendered Review of attachment 667292 [details] [diff] [review]: ----------------------------------------------------------------- r+ with that fixed ::: layout/base/nsLayoutUtils.cpp @@ +1266,5 @@ > + if (!layer || !layer->HasUserData(nsIFrame::LayerIsPrerenderedDataKey())) { > + // This layer isn't prerendered, so surrounding content may have > + // been invalidated and any "current layer transform" we compute > + // here may be bogus. > + return false; This check should be in the caller, I believe. It's not really related to the functionality of GetLayerTransformForFrame.

OK, makes sense. I guess if there's still not an invalidating paint scheduled we wouldn't change the computed matrix, even for non-prerendered.

I went ahead and moved the prerender key code back into part 4 too.

https://hg.mozilla.org/integration/mozilla-inbound/rev/abeaa961194f https://hg.mozilla.org/integration/mozilla-inbound/rev/5d983c2b4f12 https://hg.mozilla.org/integration/mozilla-inbound/rev/734431216644 https://hg.mozilla.org/integration/mozilla-inbound/rev/6eccc1276660

Backed out, because my display-list-fu is weak. https://hg.mozilla.org/integration/mozilla-inbound/rev/a2581a4bda01

Was trying to find a simple page to check that I had fixed the leak, and pulled up this one. We don't leak, but these patches cause an apparent rendering glitch; the transition doesn't seem to run. Scary that our test suite doesn't catch bugs like this.

But it works fine if I force-enable GL layers. *scratches head*

I think what's going on here is that moving the transformed frame without invalidating works fine wrt *layers*, but it's not invalidating the region of the *widget* that the transformed frame moved off of. I think this works for existing empty transactions because they only kick in for buffer-only changes, that is when there's no OS widget area to be invalidated outside of the changed buffer. I think this works with GL compositing because we recomposite the entire window every frame. Which suggests a solution.

I don't particularly like separating layout optimizations into a "is compositing cheap" bucket and not, but since it's extremely hard for us to test "not" in this case and it's going away, meh.

Comment on attachment 667389 [details] [diff] [review] Bail out if we have to worry about widget invalidation, and make the leak checker shut up Review of attachment 667389 [details] [diff] [review]: ----------------------------------------------------------------- This is fine. However, I don't know how invalidation works with empty transactions and BasicLayermanager and DLBI.

Works better, but still not quite right.

Comment on attachment 667408 [details] [diff] [review] Let DLBI compute invalid regions for non-GPU compositors Review of attachment 667408 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/Layers.h @@ +719,4 @@ > */ > void SetBaseTransformForNextTransaction(const gfx3DMatrix& aMatrix) > { > + if (mPendingTransform && *mPendingTransform == aMatrix) { You might as well just skip this test I think ::: gfx/layers/basic/BasicLayerManager.cpp @@ +562,5 @@ > } > > + // Apply pending tree updates before recomputing effective > + // properties. > + ApplyPendingUpdatesForLayerTree(aLayer); Don't you need this code in all layer managers?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #72) > Comment on attachment 667408 [details] [diff] [review] > Let DLBI compute invalid regions for non-GPU compositors > > ::: gfx/layers/basic/BasicLayerManager.cpp > @@ +562,5 @@ > > } > > > > + // Apply pending tree updates before recomputing effective > > + // properties. > > + ApplyPendingUpdatesForLayerTree(aLayer); > > Don't you need this code in all layer managers? I do now, yeah. Let me see if I can get this to happen during ComputeEffectiveTransforms() (any override) somehow.

Additionally, we're no longer Mutate()ing when these changes happen, which breaks shadow layers.

Fixes the two original bugs, and fixes the GPU manager bugs, but breaks shadow layers for a reason that I don't yet understand. Will resume tomorrow.

I forgot how to do recursion. Fixed.

This fixes shadow layers, but now I'm seeing invalidation bugs again with basic layers :(. I need another try run, so have some time, but at worst we can land with this enabled only non-clip-compositing layer managers (like above) and sort this in a followup.

The useless homepage screen has apparently come back from the dead, so panning between there and the first useful screen is back down from 60fps. Bug 796697 can fix that.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #77) > Created attachment 667576 [details] [diff] [review] > Let DLBI compute invalid regions for non-GPU compositors > > This fixes shadow layers, but now I'm seeing invalidation bugs again with > basic layers :(. I need another try run, so have some time, but at worst we > can land with this enabled only non-clip-compositing layer managers (like > above) and sort this in a followup. I only seem to be able to reproduce this when I load the testcase as |firefox file:///[test].html. I wonder if this is hitting some sort of startup bug in widget ...

Comment on attachment 667576 [details] [diff] [review] Let DLBI compute invalid regions for non-GPU compositors Review of attachment 667576 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/Layers.cpp @@ +500,5 @@ > > +void > +Layer::ApplyPendingUpdatesToSubtree() > +{ > + remove blank line @@ +504,5 @@ > + > + ApplyPendingUpdatesForThisTransaction(); > + for (Layer* next = GetNextSibling(); next; next = next->GetNextSibling()) { > + next->ApplyPendingUpdatesToSubtree(); > + } I don't get this. Shouldn't this be calling ApplyPendingUpdatesToSubtree on this layer's children, not its siblings?

I'm able to reproduce this pretty reliably now after startup by maximizing an un-maximized window (or un-maximizing a maximized one) while the animation runs. Here's the WidgetPaint log from around then 597604160[1b00810]: Invalidate (rect) [2986a40]: 0 63 400 110 597604160[1b00810]: sending expose event [2986a40] 1b565a0 0x420013e (rects follow): 597604160[1b00810]: 0 63 400 110 597604160[1b00810]: Invalidate (rect) [2986a40]: 0 63 400 107 597604160[1b00810]: sending expose event [2986a40] 1b565a0 0x420013e (rects follow): 597604160[1b00810]: 0 63 400 107 597604160[1b00810]: Invalidate (rect) [2986a40]: 0 30 1154 136 597604160[1b00810]: sending expose event [2986a40] 1b565a0 0x420013e (rects follow): 597604160[1b00810]: 0 30 1154 136 597604160[1b00810]: Invalidate (rect) [2986a40]: 0 63 400 99 597604160[1b00810]: sending expose event [2986a40] 1b565a0 0x420013e (rects follow): 597604160[1b00810]: 0 63 400 99 597604160[1b00810]: Invalidate (rect) [2986a40]: 0 63 400 96 597604160[1b00810]: sending expose event [2986a40] 1b565a0 0x420013e (rects follow): 597604160[1b00810]: 0 63 400 96 597604160[1b00810]: Invalidate (rect) [2986a40]: 0 63 400 90 597604160[1b00810]: sending expose event [2986a40] 1b565a0 0x420013e (rects follow): 597604160[1b00810]: 0 63 400 90 I think "597604160[1b00810]: 0 30 1154 136" is the OS invalidationg from the maximization. It seems to mess with the layers painting somehow.

Includes recursion fix, compile fix, and check that we're not in an aborted empty txn.

Comment on attachment 667762 [details] [diff] [review] Let DLBI compute invalid regions for non-GPU compositors, v2 Review of attachment 667762 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/Layers.cpp @@ +500,5 @@ > > +void > +Layer::ApplyPendingUpdatesToSubtree() > +{ > + Remove blank line @@ +502,5 @@ > +Layer::ApplyPendingUpdatesToSubtree() > +{ > + > + ApplyPendingUpdatesForThisTransaction(); > + for (Layer* next = GetNextSibling(); next; next = next->GetNextSibling()) { This is not correct! ::: gfx/layers/basic/BasicLayerManager.cpp @@ +533,5 @@ > > + if (aLayer) { > + // Apply pending tree updates before recomputing effective > + // properties. > + aLayer->ApplyPendingUpdatesToSubtree(); This doesn't look right. You're supposed to check this is not a composite-only transaction. ::: gfx/layers/d3d10/LayerManagerD3D10.cpp @@ +359,5 @@ > if (mRoot && !(aFlags & END_NO_IMMEDIATE_REDRAW)) { > mCurrentCallbackInfo.Callback = aCallback; > mCurrentCallbackInfo.CallbackData = aCallbackData; > > + mRoot->ApplyPendingUpdatesToSubtree(); This should match what we do in BasicLayerManager.

Comment on attachment 667762 [details] [diff] [review] Let DLBI compute invalid regions for non-GPU compositors, v2 Jeez, this is the *second* time I've reposted the same busted patch. Sorry guys.

Comment on attachment 667774 [details] [diff] [review] Let DLBI compute invalid regions for non-GPU compositors, v2 (right patch) Confirmed it's the right patch this time.

Comment on attachment 667774 [details] [diff] [review] Let DLBI compute invalid regions for non-GPU compositors, v2 (right patch) Review of attachment 667774 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/Layers.cpp @@ +678,5 @@ > +Layer::ApplyPendingUpdatesForThisTransaction() > +{ > + if (mPendingTransform && *mPendingTransform != mTransform) { > + mTransform = *mPendingTransform; > + mPendingTransform = nullptr; Shouldn't we be clearing this regardless of whether it was different?

Indeedly do.

Pushed with parts 2 and 3 swapped to make it easier for me to distribute fixes https://hg.mozilla.org/integration/mozilla-inbound/rev/e0907e4af12b https://hg.mozilla.org/integration/mozilla-inbound/rev/d333cf74425f https://hg.mozilla.org/integration/mozilla-inbound/rev/f3d44e273f4e https://hg.mozilla.org/integration/mozilla-inbound/rev/1cceedd227a7

https://hg.mozilla.org/mozilla-central/rev/e0907e4af12b https://hg.mozilla.org/mozilla-central/rev/d333cf74425f https://hg.mozilla.org/mozilla-central/rev/f3d44e273f4e https://hg.mozilla.org/mozilla-central/rev/1cceedd227a7