The default bug view has changed. See this FAQ.

Fennec invalidation regression on Nightly

VERIFIED FIXED in Firefox 17

Status

()

Core
Layout
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: jrmuizel, Assigned: cwiiis)

Tracking

({regression})

unspecified
mozilla17
ARM
Android
regression
Points:
---

Firefox Tracking Flags

(firefox17+ verified, fennec17+)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
tracking-fennec: --- → ?
Keywords: regression, regressionwindow-wanted
OS: Mac OS X → Android
Hardware: x86 → ARM
Summary: Invalidation regression on Nightly → Fennec invalidation regression on Nightly
(Reporter)

Comment 1

5 years ago
Aurora is also fine.
tracking-firefox17: --- → ?
how bad of a performance regression is this in the wild
tracking-fennec: ? → 17+
(Reporter)

Comment 3

5 years ago
(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.
(Reporter)

Updated

5 years ago
Keywords: qawanted
tracking-firefox17: ? → +
(Assignee)

Comment 4

5 years ago
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
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
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.
Attachment #653761 - Flags: review?(roc)
(Assignee)

Comment 7

5 years ago
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)
(Assignee)

Comment 8

5 years ago
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 :(
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?
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Comment 11

5 years ago
Created attachment 654146 [details] [diff] [review]
Retain DisplayItemDataEntry's for merged frames

Do as suggested in comment #9
Attachment #653801 - Attachment is obsolete: true
Attachment #653801 - Flags: review?(roc)
Attachment #654146 - Flags: review?(roc)
(Assignee)

Comment 12

5 years ago
> 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.
(Assignee)

Comment 13

5 years ago
Created attachment 654156 [details] [diff] [review]
Retain DisplayItemDataEntry's for merged frames

Sorry for churn, removed the unnecessary nsDisplayList changes.
Attachment #654146 - Attachment is obsolete: true
Attachment #654146 - Flags: review?(roc)
Attachment #654156 - Flags: review?(roc)
Attachment #654156 - Flags: review?(roc) → review+
Don't forget to update the patch commit message though!
(Assignee)

Comment 15

5 years ago
Pushed to inbound with updated commit message: https://hg.mozilla.org/integration/mozilla-inbound/rev/330c2cc06bbd
(Assignee)

Comment 16

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(Assignee)

Comment 18

5 years ago
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.
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+
(Assignee)

Comment 20

5 years ago
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/a485ff904954
https://hg.mozilla.org/mozilla-central/rev/a485ff904954

Updated

5 years ago
Status: RESOLVED → VERIFIED
status-firefox17: --- → verified

Updated

5 years ago
Keywords: qawanted
Keywords: regressionwindow-wanted
You need to log in before you can comment on or make changes to this bug.