If you open the sms app and then click "New Message" e.g., there will be a CSS transition started between the main screen and the new message screen. The fps of this on device (Galaxy S II) is very low, 5-10 fps. Paint flashing shows that we're repainting the screens like crazy. The framerate seems to inversely depend on the size of the page; for the "Erasmus Flynn" conversation view, which has many messages, the transition framerate is lower than for "Aladdin Ellison", which has fewer. I'm not sure why that is. The sms app currently puts a transition on ".left", which will definitely force reflow in general. However, switching the transition to an animated translation transform doesn't help, even though it should. The -->translation switch and bug 722923 might be enough to fix this.

> The -->translation switch and bug 722923 might be enough to fix this. Nope, still slow.

What's going on here is that the frame that we're "sliding in" is width:100% height:100% so should hit our prerender optimization. It doesn't though; paint flashing shows we repaint it on every sample of the transition. The frame has a box-shadow of radius 20px. If I remove that and replace it with a border, then we indeed hit the prerender optimization and don't invalidate while sampling the transition. I'm not sure whether the shadow is making the frame look larger than the window size and so throws out the optimization there, or whether the shadow is causing some other issue. Will poke a bit more.

(NB: I'm testing with the fix for bug 722923 applied.)

It looks like the nsDisplayTransform is counting the box shadow. So, I have a proposed fix!

This is part 1, because even with this fix something is causing the transformed frame to be invalidated.

(And yes, this fuzz factor is a slippery slope, and no, you don't want to know how I chose 1/8 ;).

The symptom here is the same as in bug 722923 void nsIFrame::InvalidateTransformLayer() { bool hasLayer = is returning false where hit, even though clearly the frame under transition is getting its own layer. Double checking that I have the patches applied correctly ...

So we're hitting bool nsIFrame::FinishAndStoreOverflow(nsOverflowAreas& aOverflowAreas, nsSize aNewSize) { //... if (hasOutlineOrEffects) { roc, anything we can do here?

Hmm, but in this case, I'm not sure that the "visual" overflow technically changes.

Comment on attachment 594550 [details] [diff] [review] Part 1: Add a little fuzz factor to the max pre-render size for transformed frames, in case the frame has borders or shadows Fix works great, thanks roc! Any happy Waitangi Day. We also need this patch for this particular test case. It doesn't make me incredibly happy, but I think a fuzz region for borders etc. is reasonable. I have no idea what the heuristics should be, but like we did with the previous patch, we can tune this more in the future.

Hmm, does this problem only occur on trunk / aurora? We have a known bug with dynamic transform style changes that was intentionally introduced by bug 722325 to fix some other (more severe) bug. It will be fixed correctly in bug 724432.

Here's a testcase to illustrate the existing bug. Try-server builds with bug 724432 fixed if you want to test it: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mpalmgren@mozilla.com-fa1a77db5788/

I've only been testing with trunk. I'll try to check the try build later tonight. In general, getting these bugs fixed is extremely high priority for us. On the test cases we've been analyzing, the performance on desktop FF goes from ~60fps *with* excessive invalidation to ~60fps with the bug fixed. But on phone hardware, performance goes from ~10fps to 60fps with the fixes. That's the difference between laughably bad and competitive.

We urgently need this bug fixed for b2g. Can we mark the dependency on 724432 and we should get reviews as quickly as we can. We have MWC coming up and performance without this patch is miserable. Thanks!

(In reply to Mats Palmgren [:mats] from comment #14) > Hmm, does this problem only occur on trunk / aurora? No, I don't think so.

I mean, I think this bug has been around as long as we've been using UpdateTransformLayer hints.

(In reply to Mats Palmgren [:mats] from comment #15) > Created attachment 594806 [details] > Another testcase with dynamic transform changes > > Here's a testcase to illustrate the existing bug. > > Try-server builds with bug 724432 fixed if you want to test it: > https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mpalmgren@mozilla. > com-fa1a77db5788/ Doesn't fix the bug here.

Mats, any ETA on review here?

Comment on attachment 594632 [details] [diff] [review] fix Did you really intend to not invalidate anything at all even though the post-transform visual overflow rect did change? That doesn't look right to me. The only reason the content is painted in its new position is the hideous hack in InvalidateInternalAfterResize that UnionRect the damage rect with its transformed self because "we don't know what coordinate space this rectangle is in". *shrug* http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#4560 To illustrate my point, this patch applies on top of yours. Load "Testcase #3" with paint flashing enabled. It shows that we invalidate the old pre-transform rect (in DoApplyRenderingChangeToTree), and the new post-transform rect. I think we should invalidate the post-transform rect in both cases, at least for UpdateOverflow. So that's two separate bugs that we should fix. Is the second hunk in my patch what you really meant? If so, r+ with that. I don't want a fix that rely on the UnionRect bug though.

Ignore the browser-thumbnails.js change, it's just to disable drawWindow() calls for thumbnails which gets in the way of debugging.

Comment on attachment 596392 [details] [diff] [review] debug patch Review of attachment 596392 [details] [diff] [review]: ----------------------------------------------------------------- You're absolutely right. This patch is correct (with the fix below). r+ with that. Fortunately, bug 539356 will make all this mess go away. ::: layout/generic/nsFrame.cpp @@ +6783,5 @@ > // changed (in particular, if it grows), we have to repaint the > // new area here. > Invalidate(aOverflowAreas.VisualOverflow()); > + } > + } else if (anyOverflowChanged && hasTransform) { this should not be "else if". It should just be "if" since we might need to invalidate along both paths.

Comment on attachment 594632 [details] [diff] [review] fix r+ with the InvalidateLayer patch

Can you land this lot? :-)

Yes, but what's the ordering of your patches? Mine shouldn't depend on yours, can go before or after.

The only dependency is that 'InvalidateLayer' must apply on top of 'fix'.

roc, mats can I get some commit messages for your patches?

Mine is just a minor correction to roc's patch, so you can just fold it into his.

Thanks, will do.

"Bug 724189. Don't invalidate entire frame when the frame has shadows or effects and the post-transform overflow area changes. r=mats"

The test here apparently depends on the test from bug 722923. Omitting for this landing.

https://hg.mozilla.org/integration/mozilla-inbound/rev/43b3ef9500fe https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f7322ed113 Leaving open to land test.

https://hg.mozilla.org/mozilla-central/rev/43b3ef9500fe https://hg.mozilla.org/mozilla-central/rev/d4f7322ed113

Transitions there are fast. Leaving the landing of the test to bug 722923.