Closed Bug 781516 Opened 7 years ago Closed 7 years ago

Fennec invalidation regression on Nightly

Categories

(Core :: Layout, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla17
Tracking Status
firefox17 + verified
fennec 17+ ---

People

(Reporter: jrmuizel, Assigned: cwiiis)

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

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.
tracking-fennec: --- → ?
OS: Mac OS X → Android
Hardware: x86 → ARM
Summary: Invalidation regression on Nightly → Fennec invalidation regression on Nightly
Aurora is also fine.
how bad of a performance regression is this in the wild
tracking-fennec: ? → 17+
(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.
Keywords: qawanted
So it looks like this is another case of two display items sharing the same frame messing up invalidations, again... Still investigating.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
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.
This fixes the issue by adding a flag for visible clip items to DisplayItemDataEntry.
Attachment #653761 - Flags: review?(roc)
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...
Attachment #653761 - Flags: review?(roc)
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 :(
Attachment #653761 - Attachment is obsolete: true
Attachment #653801 - Flags: review?(roc)
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?
(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.
Do as suggested in comment #9
Attachment #653801 - Attachment is obsolete: true
Attachment #653801 - Flags: review?(roc)
Attachment #654146 - Flags: review?(roc)
> 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.
Sorry for churn, removed the unnecessary nsDisplayList changes.
Attachment #654146 - Attachment is obsolete: true
Attachment #654146 - Flags: review?(roc)
Attachment #654156 - Flags: review?(roc)
Don't forget to update the patch commit message though!
Pushed to inbound with updated commit message: https://hg.mozilla.org/integration/mozilla-inbound/rev/330c2cc06bbd
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.
https://hg.mozilla.org/mozilla-central/rev/330c2cc06bbd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Just some clean-up, as mentioned in comment #16. Apologies for not noticing this in the first place.
Attachment #654943 - Flags: review?(roc)
Comment on attachment 654943 [details] [diff] [review]
Reeplace mIsMergedFramewith mIsSharingContainerLayer

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

My apologies for not spotting that.
Attachment #654943 - Flags: review?(roc) → review+
Status: RESOLVED → VERIFIED
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.