1205087 - Cache the AnimatedGeometryRoot on DisplayItem

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

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.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Remove LayerManager parameter for ShouldFixToViewport

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Cache AnimatedGeometryRoot

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

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8661528 [details] [diff] [review]
Cache AnimatedGeometryRoot

Review of attachment 8661528 [details] [diff] [review]:
-----------------------------------------------------------------

Please remove the other cache!!!

Comment 4

[Matt Woodrow (:mattwoodrow)]

(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.

Comment 5

[Pulsebot]

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

Comment 6

[Phil Ringnalda (:philor)]

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.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Paint continuations manually

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.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Cache AnimatedGeometryRoot v2

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 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 11

[Pulsebot]

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

Comment 12

[Matt Woodrow (:mattwoodrow)]

(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.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

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

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Kartikaya Gupta (email:[e-mail])]

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

Comment 17

[Nicholas Nethercote [inactive]]

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.

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 19

[Nicholas Nethercote [inactive]]

(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.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

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

Thanks!

Comment 21

[Timothy Nikkel (:tnikkel)]

Attachment: what's left to reland, rebased

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.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1bee32f7e25

Comment 24

[Kartikaya Gupta (email:[e-mail])]

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

Comment 25

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 26

[Matt Woodrow (:mattwoodrow)]

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 :)

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f1bee32f7e25