1222880 - Build a tree of AnimatedGeometryRoot objects to optimize finding ancestors

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

FLB frequently walks up AnimatedGeometryRoots (usually looking for scrollable frames), and this adds up to a fair amount of time in some profiles.

We already basically have this information during display list building as AutoBuildingDisplayList stores each AnimatedGeometryRoot on the stack. I want to copy this into the display list arena so that FLB can use it.

Comment 1

[Matt Woodrow (:mattwoodrow)]

I also want to get rid of the nsLayoutUtils functions for finding AGRs, and require that you obtain them from a display item instead.

We had confusion previously where the AGR for a frame wasn't always defined (transforms, background-attachment:fixed etc), and this should fix that.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: [WIP] first attempt

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: [WIP] passes most tests

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Keep a tree of AGRs for faster traversal

Comment 5

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

Comment on attachment 8685935 [details] [diff] [review]
Keep a tree of AGRs for faster traversal

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

::: layout/base/nsDisplayList.cpp
@@ +669,5 @@
>                             getter_AddRefs(mBoundingSelection));
>      }
>    }
>  
> +  mAnimatedGeometryRootCache.Put(aReferenceFrame, &mRootAGR);

Call this mFrameToAnimatedGeometryRootMap to indicate it's not a cache.

::: layout/base/nsDisplayList.h
@@ +107,5 @@
>    virtual const char* Name() override { return n; } \
>    virtual Type GetType() override { return e; }
>  
> +
> +struct AnimatedGeometryRoot

Needs documentation

@@ +1122,5 @@
>  
>      uint32_t mBudget;
>    };
>  
> +  nsIFrame* const                      mReferenceFrame;

fix indent, or just stop aligning all these fields

Comment 6

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8685935 [details] [diff] [review]
Keep a tree of AGRs for faster traversal

Since you are removing the only user of nsDisplayTransform::IsReferenceFrameBoundary can you remove that function too?

So this effectively removes the frame->AGR cache, and replaces it with an frameAGR->AGR struct cache. Why not just make it a frame->AGR struct cache?

>+nsDisplayListBuilder::WrapAGRForFrame(nsIFrame* aAnimatedGeometryRoot,
>+      nsIFrame* parentFrame = nsLayoutUtils::GetCrossDocParentFrame(aAnimatedGeometryRoot);

Can you add an assert that this doesn't go above the root reference frame?

> nsDisplayListBuilder::RecomputeCurrentAnimatedGeometryRoot()
> {
>+  if (*mCurrentAGR != mCurrentFrame &&
>+      IsAnimatedGeometryRoot(const_cast<nsIFrame*>(mCurrentFrame))) {
>+    mCurrentAGR = WrapAGRForFrame(const_cast<nsIFrame*>(mCurrentFrame), mCurrentAGR);
>+  }
> }

This technically isn't enough since any child AGR struct of mCurrentFrame will point to mCurrentAGR when it should point to mCurrentFrame.

It's gross, but should we iterate the hash table of AGR structs and update their fields?

>+struct AnimatedGeometryRoot
>+  AnimatedGeometryRoot* mParent;

Can we call this mParentAGR? Otherwise it's too easy to get confused between the AGR parent and the frame parent.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Keep a tree of AGRs for faster traversal v2

Comment 8

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8691270 [details] [diff] [review]
Keep a tree of AGRs for faster traversal v2

Thanks.

We'll have to keep on eye on RecomputeCurrentAnimatedGeometryRoot because if we force activate a scrollframe that is deeply nested inside other scrollframes we'll have to force activate them all, and call RecomputeCurrentAnimatedGeometryRoot for them all. But that should be rare (because we set a displayport on all ancestors when setting a displayport on a child, and it should only last one paint because we set a displayport when we force layerize.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78381aa444d4

Comment 10

[Pulsebot]

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

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e8bd912ef98

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/78381aa444d4
https://hg.mozilla.org/mozilla-central/rev/dd4e51f9f4aa
https://hg.mozilla.org/mozilla-central/rev/5e8bd912ef98