We have a hashtable based cache already, but it doesn't seem to work all that well. FrameLayerBuilder frequently spends large amounts of time finding the animated geometry roots for display items. This really shows up for bug 1204550.

This drops the 'idle' cpu usage on reddit.com/r/space from ~45% to ~38% (while maintaining 60fps).

Comment on attachment 8661528 [details] [diff] [review] Cache AnimatedGeometryRoot Review of attachment 8661528 [details] [diff] [review]: ----------------------------------------------------------------- Please remove the other cache!!!

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3) > Comment on attachment 8661528 [details] [diff] [review] > Cache AnimatedGeometryRoot > > Review of attachment 8661528 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please remove the other cache!!! We still call GetAnimatedGeometryRootForFrame from other places, so it's helping us there. Just not as effectively as being to direct access a pointer.

https://hg.mozilla.org/integration/mozilla-inbound/rev/11a3cb42f729 https://hg.mozilla.org/integration/mozilla-inbound/rev/0573cd4aed27

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/91267a9f8b5f for... https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=0573cd4aed27&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception - if I tried to tell you all the things, I'd be bound to miss several of them.

Trying to build a single display list for all continuations means that the RootReferenceFrame() for the nsDisplayListBuilder wasn't an ancestor of all frames used in the display list. This breaks our attempts to find AGR/reference frames since we walk up the frame tree until we find one or hit the root. We were ending up picking a frame in the parent document as our AGR.

This fixes all the bugs in our existing AGR lookups and adds more assertions (that now pass). In particular it gets rid of the aStopAtAncestor, and makes sure that all ReferenceFrames are also counted as AGRs (which was a poorly enforced requirement previously). The problem with have aStopAncestor was when we passed the 'parent' AGR as this parameter, but the current frame is position:fixed. In this case the aStopAncestor wasn't in the ancestor chain for the current frame, and as a result we could walk up past the root reference frame and into a parent document.

Comment on attachment 8665668 [details] [diff] [review] Paint continuations manually Review of attachment 8665668 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/svg/nsSVGIntegrationUtils.cpp @@ +681,5 @@ > nsRenderingContext context(aContext); > nsLayoutUtils::PaintFrame(&context, mFrame, > dirty, NS_RGBA(0, 0, 0, 0), > flags); > + trailing whitespace

Comment on attachment 8665670 [details] [diff] [review] Cache AnimatedGeometryRoot v2 Review of attachment 8665670 [details] [diff] [review]: ----------------------------------------------------------------- Can you check how effective the other cache is? If it's not significant we should take it out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4d65450f87c https://hg.mozilla.org/integration/mozilla-inbound/rev/6de2f629a5d0 https://hg.mozilla.org/integration/mozilla-inbound/rev/5732108028dd

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > Comment on attachment 8665670 [details] [diff] [review] > Cache AnimatedGeometryRoot v2 > > Review of attachment 8665670 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can you check how effective the other cache is? If it's not significant we > should take it out. Yes, will do.

The patches on this bug cause test failures with APZ enabled. At the very least the M-4 failure (test_layerization.html) seen in https://treeherder.mozilla.org/#/jobs?repo=try&revision=344c1090f7de wasn't happening without these patches, and is happening with these patches. I suspect the reftest failures are also a result of these patches although I haven't verified that.

Also the part 2 patch doesn't build on its own as it uses AnimatedGeometryRoot() which is only added in part 3.

https://hg.mozilla.org/mozilla-central/rev/e4d65450f87c https://hg.mozilla.org/mozilla-central/rev/6de2f629a5d0 https://hg.mozilla.org/mozilla-central/rev/5732108028dd

Sorry, I decided to back out the AGR cache on nsDisplayItem for causing bug 1208780 which is a real-world regression. See bug 1208780 comment 6 for details; I'll work on a fix for the regression and try to get this re-landed as soon as possible. https://hg.mozilla.org/integration/mozilla-inbound/rev/15ed42bf5d4c

FYI: the following code that mattwoodrow removed and then kats added back in is buggy: > PLDHashNumber Hash() const { > return mozilla::HashBytes(this, sizeof(this)); > } It should be |sizeof(*this)|. This won't affect correctness, it just means it's a bad hash function that could have lots of collisions -- it'll hash |mFrame| which is the first field, but won't hash |mStopAtFrame|. It's possible this is part of the reason why the cache doesn't work well, as comment 0 describes.

when this lands again we will see 2 improvements (the backout caused these two regressions): http://alertmanager.allizom.org:8080/alerts.html?rev=15ed42bf5d4c132adad9f34c3985a0b310a35d42&showAll=1&testIndex=0&platIndex=0

(In reply to Nicholas Nethercote [:njn] from comment #17) > FYI: the following code that mattwoodrow removed and then kats added back in > is buggy: > > > PLDHashNumber Hash() const { > > return mozilla::HashBytes(this, sizeof(this)); > > } > > It should be |sizeof(*this)|. This won't affect correctness, it just means > it's a bad hash function that could have lots of collisions -- it'll hash > |mFrame| which is the first field, but won't hash |mStopAtFrame|. I just fixed this in bug 1216386.

(In reply to Nicholas Nethercote [:njn] from comment #19) > > I just fixed this in bug 1216386. Thanks!

This is what's left of the things backed out that haven't relanded so far after all the churn I did. One notable change required: needed to add nsLayoutUtils::GetAnimatedGeometryRootForInit that can be called from nsDisplayItem constructor. I changed nsLayoutUtils::GetAnimatedGeometryRootFor to call GetType on the item to handle transform items.

^ I probably shouldn't have been listed as the author of that patch...

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24) > ^ I probably shouldn't have been listed as the author of that patch... Oops, I originally pulled the "reland what I (kats) backed out" patch from one of your try server pushes, and I guess I just kept updating that patch in my patch queue the whole time.

Double oops, I fixed the commit message and forgot to check the author. Oh well, you can redirect people to me if there's any fallout :)