Closed Bug 1205087 Opened 7 years ago Closed 7 years ago

Cache the AnimatedGeometryRoot on DisplayItem

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 --- affected
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files, 1 obsolete file)

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.
Attached patch Cache AnimatedGeometryRoot (obsolete) — Splinter Review
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+
(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.
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)
Attachment #8661528 - Attachment is obsolete: true
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+
(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.
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 → ---
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.
(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.
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.
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 :)
https://hg.mozilla.org/mozilla-central/rev/f1bee32f7e25
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: mozilla44 → mozilla45
You need to log in before you can comment on or make changes to this bug.