Last Comment Bug 781516 - Fennec invalidation regression on Nightly
: Fennec invalidation regression on Nightly
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: mozilla17
Assigned To: Chris Lord [:cwiiis]
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-09 07:19 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2013-06-13 11:55 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
17+


Attachments
Page with bad invalidation behaviour (6.53 KB, text/html)
2012-08-09 07:19 PDT, Jeff Muizelaar [:jrmuizel]
no flags Details
Fix invalidation on clip items with an underlying frame that has been merged into a container frame (7.15 KB, patch)
2012-08-21 08:21 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Fix invalidation on clip items with an underlying frame that has been merged into a container frame (v2) (9.50 KB, patch)
2012-08-21 09:43 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Retain DisplayItemDataEntry's for merged frames (7.93 KB, patch)
2012-08-22 02:23 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Retain DisplayItemDataEntry's for merged frames (6.10 KB, patch)
2012-08-22 03:24 PDT, Chris Lord [:cwiiis]
roc: review+
Details | Diff | Splinter Review
Reeplace mIsMergedFramewith mIsSharingContainerLayer (5.22 KB, patch)
2012-08-24 00:01 PDT, Chris Lord [:cwiiis]
roc: review+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-08-09 07:19:27 PDT
Created attachment 650546 [details]
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] 2012-08-09 07:28:54 PDT
Aurora is also fine.
Comment 2 Brad Lassey [:blassey] (use needinfo?) 2012-08-09 10:05:51 PDT
how bad of a performance regression is this in the wild
Comment 3 Jeff Muizelaar [:jrmuizel] 2012-08-09 10:19:57 PDT
(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] 2012-08-20 12:42:47 PDT
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] 2012-08-21 06:57:28 PDT
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] 2012-08-21 08:21:16 PDT
Created attachment 653761 [details] [diff] [review]
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] 2012-08-21 08:55:08 PDT
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] 2012-08-21 09:43:44 PDT
Created attachment 653801 [details] [diff] [review]
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) 2012-08-21 15:05:56 PDT
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] 2012-08-22 02:01:30 PDT
(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] 2012-08-22 02:23:17 PDT
Created attachment 654146 [details] [diff] [review]
Retain DisplayItemDataEntry's for merged frames

Do as suggested in comment #9
Comment 12 Chris Lord [:cwiiis] 2012-08-22 02:50:36 PDT
> 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] 2012-08-22 03:24:28 PDT
Created attachment 654156 [details] [diff] [review]
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) 2012-08-22 16:54:34 PDT
Don't forget to update the patch commit message though!
Comment 15 Chris Lord [:cwiiis] 2012-08-23 01:31:51 PDT
Pushed to inbound with updated commit message: https://hg.mozilla.org/integration/mozilla-inbound/rev/330c2cc06bbd
Comment 16 Chris Lord [:cwiiis] 2012-08-23 11:17:53 PDT
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] 2012-08-23 19:21:52 PDT
https://hg.mozilla.org/mozilla-central/rev/330c2cc06bbd
Comment 18 Chris Lord [:cwiiis] 2012-08-24 00:01:35 PDT
Created attachment 654943 [details] [diff] [review]
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) 2012-08-24 03:36:18 PDT
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] 2012-08-24 04:01:16 PDT
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/a485ff904954
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-08-24 20:03:35 PDT
https://hg.mozilla.org/mozilla-central/rev/a485ff904954

Note You need to log in before you can comment on or make changes to this bug.