805343 - Constant excessive invalidation on Tomshardware.com (Invisible animation gif causes high CPU usage and constant invalidation)

Status: REOPENED

(Core :: Layout, defect)

Description

[Mayank Bansal]

Attachment: bug.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0
Build ID: 20121024030643

Steps to reproduce:

1. enable nglayout.debug.paint_flashing
2. go to http://www.tomshardware.com/  .  Wait till page loads completely.
3. Notice that "latest news"  , "forum live feed" and "Tom's Hardware System Builder"  columns/sections are constantly flashing.
4.Scroll  the page a little as shown in the attached image (till the   "forum live feed" column covers a lot of the screen)

5. Check the high CPU usage.

This is on a new profile with no addons.


Actual results:

High CPU usage and constant invalidation of big columns.  Bigger the flashing area, worse the performance.


Expected results:

minimal invalidation.
Attaching about:support.

Profile : http://people.mozilla.com/~bgirard/cleopatra/?report=1b258462e52b8a27f35e132e93de0d4cac1ca9c5

Comment 1

[Mayank Bansal]

Attachment: about:support

Comment 2

[Alice0775 White]

I can confirm.
High CPU usage and constant invalidation happens even if disabled JavaScript

Comment 3

[Alice0775 White]

Aha,  background image throbber causes this.
The offending css is in homepage_3_min.css:
.ajaxLoader,.ui-dialog .ajaxLoader{background-image:url(http://m.bestofmedia.com/sfp/images/design/ajax/loading.gif);background-position:center center;background-repeat:no-repeat;background-color:#fff;}

The following css helps to stop High CPU usage and constant invalidation.
.ajaxLoader{background-image:none !important;}

So, I think this is the site problem.

Comment 4

[Alice0775 White]

Attachment: testcase 1 (reduced; uses image on 3rd-party server)

Comment 5

[Mayank Bansal]

If this is a site problem (and not correctable by firefox), should i contact the editor-in-chief of tomshardware regarding this ?

Comment 6

[Alice0775 White]

(In reply to mayankleoboy1 from comment #5)
> If this is a site problem (and not correctable by firefox), should i contact
> the editor-in-chief of tomshardware regarding this ?

However, Firefox has to treat invisible animation gif properly.
I.e. invisible animation gif should not affect CPU usage and invalidation.

Comment 7

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

DLBI should have/could have fixed this. Not sure why it did not.

Comment 8

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

The problem is that the display item for the animated background image that's covered by the background color is preserved in the display list processed by ContainerState::ProcessDisplayItems. We don't cull hidden items up to that point, because we need to be able to render those items if they end up in a different ThebesLayer to the item(s) covering them.

Comment 9

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

This is kinda hard to fix. We compute visibility of display items twice:
1) When optimizing the display list for the full window. At this point we can't discard totally invisible items, because they may need to drawn into their ThebesLayer when the stuff that covers them falls into another layer.
2) In DrawThebesLayer, when determining which parts of display items need to be drawn. However, an item whose visible rect is empty here just means that it didn't intersect the region to draw --- it may in fact be visible, but entirely retained by the ThebesLayer so it doesn't need to be redrawn.

Comment 10

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

So in neither of those cases is it generally OK to treat the display item as totally hidden so we can suppress invalidation of it later.

Comment 11

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

Attachment: possible fix

Tell me what you think of this.

This fixes the attached testcase, and some of Tom's Hardware, but not the "Forum Live Feed", which is still invalidating constantly. I think that's something to do with region calculations ... need more time to figure it out.

Comment 12

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

Attachment: Part 2: Update HasRetainedDataFor

Comment 13

[Matt Woodrow (:mattwoodrow)]

The iterator changes look good, but I'm not a fan of having to compute this in DrawThebesLayer.

Don't we know the same information in FindThebesLayerFor/Accumulate? We could return a visible rect from this, and pass it into InvalidateForLayerChange and restrict invalidation to that area.

That would also fix the equivalent bug using a style change instead of an animated image.

We still might want something extra to avoid the SchedulePaint in this case too.

Comment 14

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> Don't we know the same information in FindThebesLayerFor/Accumulate? We
> could return a visible rect from this, and pass it into
> InvalidateForLayerChange and restrict invalidation to that area.

Accumulate builds an opaque region from back to front. To compute visibility we have to iterate from front to back. So I don't see how to leverage Accumulate here.

> That would also fix the equivalent bug using a style change instead of an
> animated image.

I'm not sure what you mean by this.

Comment 15

[Matt Woodrow (:mattwoodrow)]

> Accumulate builds an opaque region from back to front. To compute visibility
> we have to iterate from front to back. So I don't see how to leverage
> Accumulate here.

So it does. For some reason I thought it was doing front to back.

> 
> > That would also fix the equivalent bug using a style change instead of an
> > animated image.
> 
> I'm not sure what you mean by this.

Don't we have a similar bug where an entirely obscured display item that gets a style change will trigger unnecessary invalidation?

Or a partially obscured display item will trigger invalidation for the covered pixels.

Comment 16

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

Yes, I see. I don't know if those cases are important though.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 685575 [details] [diff] [review]
possible fix

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

Alright, lets just assume that it's not a big problem for the moment. We can worry about it later if it shows up.

::: layout/base/FrameLayerBuilder.cpp
@@ +3245,5 @@
> +    }
> +
> +    nsRegion r;
> +    r.And(toDraw, cdi->mItem->GetVisibleRect());
> +    cdi->mToDraw = r.GetBounds();

This could just be a bool right? No point computing and storing a rect when we don't use it.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e8f5182d94c5
https://hg.mozilla.org/mozilla-central/rev/3ba90f85653a

Comment 19

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

This is mostly fixed but I think we still have constant excessive invalidation on one particular pane of tomshardware.com, so we'll need a new bug for that.

Comment 20

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2fd849f4aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/86e4243c2482

Backed out for causing bug 820603.

Comment 21

[Julien Wajsberg [:julienw]]

I don't see such invalidation on current Nightly - yet I see little invalidation squares, likely because of the hidden animated gif, in the middle of "latest news", "forum live feed" and "Tom's hardware system builder".

The CPU usage is OK.

Comment 22

[Wayne Mery (:wsmwk)]

(In reply to Julien Wajsberg [:julienw] (PTO -> 2016) from comment #21)
> I don't see such invalidation on current Nightly - yet I see little
> invalidation squares, likely because of the hidden animated gif, in the
> middle of "latest news", "forum live feed" and "Tom's hardware system
> builder".
> 
> The CPU usage is OK.

mayankleoboym do you agree?

Comment 23

[Mayank Bansal]

yes

the testcase, and tomshardware.com still reproduce the behaviour. BUT, the CPU use is quite low.
I will leave it to the devs to decide whether this is worth pursuing now or not.

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: loading gif (for use in testcase)

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (uses bugzilla-hosted image)

Here's the same testcase again, but with a bugzilla-hosted copy of the GIF, in case the original 3rd-party copy disappears someday.