781516 - Fennec invalidation regression on Nightly

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Page with bad invalidation behaviour

The attached html file causes full page invalidations (which you can see if you turn on paintflashing) on Nightly. Beta has much better invalidation behaviour.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Aurora is also fine.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

how bad of a performance regression is this in the wild

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Brad Lassey [:blassey] from comment #2)
> how bad of a performance regression is this in the wild

Depends how many sites it affects. I noticed something similar on the google search results. If it's wide spread it will hurt our performance pretty badly.

Comment 4

[Chris Lord [:cwiiis]]

So it looks like this is another case of two display items sharing the same frame messing up invalidations, again... Still investigating.

Comment 5

[Chris Lord [:cwiiis]]

This is a tricky one. With this page, we have an unoptimised display-list that looks like this:

ScrollLayer (A)
  Clip (0)
    CanvasBackground(A)
ScrollLayer (C)
  Clip (C)
    blah

That ends up optimising to:

ScrollLayer (A)
  Clip (C)
    CanvasBackground(A)

The problem is that frame C ends up in the merged frames of the ScrollLayer, but it's represented by a display item that's never processed (ProcessDisplayItems skips over clip items), so its display item data is empty and it gets removed in UpdateDisplayItemDataForFrame.

Working on a fix.

Comment 6

[Chris Lord [:cwiiis]]

Attachment: Fix invalidation on clip items with an underlying frame that has been merged into a container frame

This fixes the issue by adding a flag for visible clip items to DisplayItemDataEntry.

Comment 7

[Chris Lord [:cwiiis]]

Comment on attachment 653761 [details] [diff] [review]
Fix invalidation on clip items with an underlying frame that has been merged into a container frame

This isn't quite good enough, the clip item can be merged and end up on a different frame. Need to check the clip item's merged frames. New patch incoming...

Comment 8

[Chris Lord [:cwiiis]]

Attachment: Fix invalidation on clip items with an underlying frame that has been merged into a container frame (v2)

This patch changes nsDisplayClip(RoundedRect) to track merged frames, and sets the flag on merged frames as well as the underlying frame. This still fixes the attached page, but also improves behaviour on other pages that exhibit this bug in different ways.

There are, unfortunately, still more invalidation regressions to track down :(

Comment 9

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

Comment on attachment 653801 [details] [diff] [review]
Fix invalidation on clip items with an underlying frame that has been merged into a container frame (v2)

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

This is a little bit indirect. Instead of marking the DisplayItemDataEntry as having clip items (which isn't really why we want to preserve its layer information), we should mark the entry as being part of a merged set of frames (for the ScrollLayer, in your example), which is why we then need to preserve layer metadata in UpdateDisplayItemDataForFrame. Does that make sense?

::: layout/base/FrameLayerBuilder.h
@@ +529,5 @@
>      nsAutoTArray<DisplayItemData, 1> mData;
>      nsRefPtr<RefCountedRegion> mInvalidRegion;
>      PRUint32 mContainerLayerGeneration;
>      bool mIsSharingContainerLayer;
> +    bool mHasClipItem;

Don't you need to set mHasClipItem to false in the DisplayItemDataEntry(nsIFrame*) constructor?

Comment 10

[Chris Lord [:cwiiis]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Comment on attachment 653801 [details] [diff] [review]
> Fix invalidation on clip items with an underlying frame that has been merged
> into a container frame (v2)
> 
> Review of attachment 653801 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a little bit indirect. Instead of marking the DisplayItemDataEntry
> as having clip items (which isn't really why we want to preserve its layer
> information), we should mark the entry as being part of a merged set of
> frames (for the ScrollLayer, in your example), which is why we then need to
> preserve layer metadata in UpdateDisplayItemDataForFrame. Does that make
> sense?
> 
> ::: layout/base/FrameLayerBuilder.h
> @@ +529,5 @@
> >      nsAutoTArray<DisplayItemData, 1> mData;
> >      nsRefPtr<RefCountedRegion> mInvalidRegion;
> >      PRUint32 mContainerLayerGeneration;
> >      bool mIsSharingContainerLayer;
> > +    bool mHasClipItem;
> 
> Don't you need to set mHasClipItem to false in the
> DisplayItemDataEntry(nsIFrame*) constructor?

Both make sense - but I was thinking, a simpler and more reliable way to fix this would be to just not add invisible items in StoreNewDisplayItemData?

I'll upload patches for both in case there's a reason why that isn't an acceptable solution.

Comment 11

[Chris Lord [:cwiiis]]

Attachment: Retain DisplayItemDataEntry's for merged frames

Do as suggested in comment #9

Comment 12

[Chris Lord [:cwiiis]]

> Both make sense - but I was thinking, a simpler and more reliable way to fix
> this would be to just not add invisible items in StoreNewDisplayItemData?

Actually, this won't work what with the invalidation happening prior to this phase.

Comment 13

[Chris Lord [:cwiiis]]

Attachment: Retain DisplayItemDataEntry's for merged frames

Sorry for churn, removed the unnecessary nsDisplayList changes.

Comment 14

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

Don't forget to update the patch commit message though!

Comment 15

[Chris Lord [:cwiiis]]

Pushed to inbound with updated commit message: https://hg.mozilla.org/integration/mozilla-inbound/rev/330c2cc06bbd

Comment 16

[Chris Lord [:cwiiis]]

Realised this patch is just adding unnecessary duplication... mIsSharingContainerLayer serves the exact same purpose as mIsMergedFrame, it just needs to be checked for in UpdateDisplayItemDataForFrame. Will attach a follow-up patch tomorrow.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/330c2cc06bbd

Comment 18

[Chris Lord [:cwiiis]]

Attachment: Reeplace mIsMergedFramewith mIsSharingContainerLayer

Just some clean-up, as mentioned in comment #16. Apologies for not noticing this in the first place.

Comment 19

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

Comment on attachment 654943 [details] [diff] [review]
Reeplace mIsMergedFramewith mIsSharingContainerLayer

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

My apologies for not spotting that.

Comment 20

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/a485ff904954

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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