Cache the AnimatedGeometryRoot on DisplayItem

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox44 affected, firefox45 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8661526 [details] [diff] [review]
Remove LayerManager parameter for ShouldFixToViewport
Attachment #8661526 - Flags: review?(roc)
(Assignee)

Comment 2

3 years ago
Created attachment 8661528 [details] [diff] [review]
Cache AnimatedGeometryRoot

This drops the 'idle' cpu usage on reddit.com/r/space from ~45% to ~38% (while maintaining 60fps).
Attachment #8661528 - Flags: review?(roc)
Comment on attachment 8661528 [details] [diff] [review]
Cache AnimatedGeometryRoot

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

Please remove the other cache!!!
Attachment #8661528 - Flags: review?(roc) → review+
(Assignee)

Comment 4

3 years ago
(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.
(Assignee)

Comment 7

3 years ago
Created attachment 8665668 [details] [diff] [review]
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.
Attachment #8665668 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Attachment #8661528 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Created attachment 8665670 [details] [diff] [review]
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.
Attachment #8665670 - Flags: review?(roc)
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
Attachment #8665668 - Flags: review?(roc) → review+
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.
Attachment #8665670 - Flags: review?(roc) → review+
(Assignee)

Comment 12

3 years ago
(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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
status-firefox44: fixed → affected
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!
Created attachment 8683408 [details] [diff] [review]
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.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1102676
^ 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.
(Assignee)

Comment 26

3 years ago
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f1bee32f7e25
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: mozilla44 → mozilla45
You need to log in before you can comment on or make changes to this bug.