786064 - IE10 Chalkboard demo destroys us

Status: RESOLVED WORKSFORME

(Core :: Graphics, defect)

Description

[(no longer active)]

See the attached profile. http://people.mozilla.com/~bgirard/cleopatra/?report=fa256c17997e0804e4e1ed21c22d91cfb7a9cca2

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

All the time is in actually rendering the SVG image, as kind of expected.  (All this test does is animate scaling/moving around a <img src="Chalkboard.svg">)

- Does mozilla::image::VectorImage cache?
- Does mozilla::image::VectorImage::Draw set a clip (or already have one set)?
- Does gfxUtils::CreateSamplingRestricedDrawable (sic) create an appropriately-sized drawable when scaling up, ideally with a clip?  Are we always rendering the image at full (scaled) size?

Comment 4

[Matthias Versen [:Matti]]

There is a huge performance regression for this test with this regression range:

Last good nightly: 2012-08-01
First bad nightly: 2012-08-02

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=582d4c67b3a7&tochan
ge=588424024294

Comment 5

[Alice0775 White]

80sec
IE10

230sec
http://hg.mozilla.org/releases/mozilla-2.0/rev/fca718600ca0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1 ID:20110413222027

230sec
http://hg.mozilla.org/releases/mozilla-release/rev/3ded311d93ad
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:5.0.1) Gecko/20100101 Firefox/5.0.1 ID:20110707182747

230sec
http://hg.mozilla.org/releases/mozilla-release/rev/5b6c2f8ff6da
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0.2) Gecko/20100101 Firefox/6.0.2 ID:20110902133214

229sec
http://hg.mozilla.org/releases/mozilla-release/rev/58f3edbff1b9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0.1) Gecko/20100101 Firefox/7.0.1 ID:20110928134238

230sec
http://hg.mozilla.org/releases/mozilla-release/rev/d03b51a9b2bd
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20100101 Firefox/8.0 ID:20111104165243

229sec
http://hg.mozilla.org/releases/mozilla-release/rev/c4405d7a95f6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0.1) Gecko/20100101 Firefox/9.0.1 ID:20111220165912

230sec
http://hg.mozilla.org/releases/mozilla-release/rev/72ad46d416ce
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0.2) Gecko/20100101 Firefox/10.0.2 ID:20120215223356

230sec
http://hg.mozilla.org/releases/mozilla-release/rev/b967d9c07377
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20100101 Firefox/11.0 ID:20120312181643

228sec
http://hg.mozilla.org/releases/mozilla-release/rev/a294a5b4f12d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0 ID:20120420145725

212sec
http://hg.mozilla.org/releases/mozilla-release/rev/f48d675ffa9f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0.1 ID:20120614114901

212sec
http://hg.mozilla.org/releases/mozilla-release/rev/e5728a4e106c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20100101 Firefox/14.0.1 ID:20120713134347

216sec
http://hg.mozilla.org/releases/mozilla-release/rev/0b774a1067fe
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0.1 ID:20120905151427

233sec
http://hg.mozilla.org/releases/mozilla-release/rev/e0c8343d2809
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0 ID:20121024073032

662sec
http://hg.mozilla.org/releases/mozilla-release/rev/c23c45132139
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20100101 Firefox/17.0 ID:20121128204232

690sec
http://hg.mozilla.org/releases/mozilla-release/rev/c4fe2b219941
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 ID:20130201065344

787sec
http://hg.mozilla.org/releases/mozilla-beta/rev/e815122c4b1f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0 ID:20130130080006

714sec
http://hg.mozilla.org/releases/mozilla-aurora/rev/ed6e1f9f2243
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20130205 Firefox/20.0 ID:20130205042019

719sec
http://hg.mozilla.org/mozilla-central/rev/2360c3c46aca
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130205 Firefox/21.0 ID:20130205031033



Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/7fe96a690596
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120801145138
Bad:
http://hg.mozilla.org/mozilla-central/rev/61d7f15ea6a7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120801191939
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7fe96a690596&tochange=61d7f15ea6a7

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2f36e642ef8d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120801123138
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/f0b216d9f512
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120801131138
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2f36e642ef8d&tochange=f0b216d9f512

Triggered by:
  7198b3ea1763	Nicholas Cameron — Bug 779001. Falling back to Cairo for large canvases. r=roc

Comment 6

[Mayank Bansal]

6.91 seconds for me on IE10, with HWA on....

Comment 7

[Alice0775 White]

(In reply to mayankleoboy1 from comment #6)
> 6.91 seconds for me on IE10, with HWA on....

Nonsense. It is only the different result of a different machine.

Comment 8

[Mark Straver]

So, question, what was bug#779001 actually for? the bug itself doesn't really say anything, and the result obviously impacts more than just the testdrive (I only went there after seeing odd slowdowns in other canvas games, to find out which test would be an issue).

Comment 9

[Mayank Bansal]

(In reply to Alice0775 White from comment #7)
> (In reply to mayankleoboy1 from comment #6)
> > 6.91 seconds for me on IE10, with HWA on....
> 
> Nonsense. It is only the different result of a different machine.

Of course. 
And getting 559 seconds with nightly.

Comment 10

[Mark Straver]

Scoobi: this impacts all versions since v17.0 - Should be Trunk, right?

Comment 11

[Scoobidiver (away)]

(In reply to Mark Straver from comment #10)
> Scoobi: this impacts all versions since v17.0 - Should be Trunk, right?
It depends on the meaning you give to the Version field. For me, if it's a regression, it's the first version where it appeared. You can assess easily how long it takes to fix a bug and you don't need to add a new affected version flag every six weeks.
Trunk is OK when it's not a regression.

Comment 12

[Nick Cameron [:nrc]]

(In reply to Mark Straver from comment #8)
> So, question, what was bug#779001 actually for? the bug itself doesn't
> really say anything, and the result obviously impacts more than just the
> testdrive (I only went there after seeing odd slowdowns in other canvas
> games, to find out which test would be an issue).

I forget exactly what that bug was fixing (will teach me to write better bug descriptions). From the title it sounds like our fallback from D2D to Cairo was busted. The issue there is that with surfaces larger than the maximum texture size we cannot use HWA and so we must use software rendering. I believe this wasn't being checked correctly and bug 779001 fixed that.

Alice0775 - was the demo actually being rendered in the pre-779001 versions?

I suspect that the SVG image is being rendered to a very large surface and forcing us to fallback to the software path. If that is the case then there is no easy fix. I'm not sure what happens to the SVG image, if it is rendered as an image then presumably we would need tiled images (+ image layers?). If it is rendered as a Thebes layer, then we just need tiled Thebes layers on Windows, which is in the pipeline, but quite far off. An easy test would be to see how this looks on mobile, we already use tiled Thebes layers there. I assume IE must be doing some kind of tiling to keep using HWA.

Comment 13

[Matthias Versen [:Matti]]

>was the demo actually being rendered in the pre-779001 versions?
Yes, it looked ok on my system during my testing with the faster versions.

Comment 14

[Alice0775 White]

(In reply to Nick Cameron [:nrc] from comment #12)
> Alice0775 - was the demo actually being rendered in the pre-779001 versions?
The rendering of both does not seem to have any difference

Comment 15

[Nick Cameron [:nrc]]

Then I do not really understand what is going on - if the patch in 779001 is making a difference (which it looks like it is) then the only thing that could be different is that |surf->CairoStatus()| is falsey, which, as I understand it, means things should not render. Perhaps we are hitting that code for a different surface and the main one is rendering fine? That would be weird. Or perhaps I'm making a bad assumption about CairoStatus.

My money is still on fallback due to large surfaces, but I've no idea how or why...

Comment 16

[Mark Straver]

Since the reason for the bug is unsure, I made a quick rebuild of my local FF19b4-based setup with Bug 779001 backed out and everything renders just fine (and the browser hang as indicated in Bug 838479 is gone).

Chalkboard test renders in about 50s for me with Bug 779001 gone. 

Looking at the patch, apart from surf->CairoStatus() the only other thing I see that might be an issue is return surf.forget(); instead of return surf; -- I don't know what forget() does exactly; is that a destructor? If so, would that cause issues?

Comment 17

[Nick Cameron [:nrc]]

(In reply to Mark Straver from comment #16)
> Since the reason for the bug is unsure, I made a quick rebuild of my local
> FF19b4-based setup with Bug 779001 backed out and everything renders just
> fine (and the browser hang as indicated in Bug 838479 is gone).
> 
> Chalkboard test renders in about 50s for me with Bug 779001 gone. 

We need to know exactly what is going on - whether surf->CairoStatus() is non-zero (I assume it must be) and why, and for which surface. Without that info it's hard to make any further progress. As far as I can see, bug 779001 is correct and fixed some real issues, backing it out is not an option.

One thing that could be broken is if calling CairoStatus() on a d2d surface is non-zero when the surface is OK, that would cause this kind of perf regression and be really, really bad in general. Looking at the code, that should only happen if mSurface is bad, and that shouldn't happen, so more mystery.

> 
> Looking at the patch, apart from surf->CairoStatus() the only other thing I
> see that might be an issue is return surf.forget(); instead of return surf;
> -- I don't know what forget() does exactly; is that a destructor? If so,
> would that cause issues?

That is because surf is changed to a RefPtr, forget() drops a reference and makes a temporary reference to surf. I believe it is correct and should have only a very minimal perf impact (using ref counting is slower than raw pointers, but only ever so slightly).

Comment 18

[Mark Straver]

I can confirm the RefPtr construction is not the issue here, it's one of the two CairoStatus() checks, indeed. I haven't had the time (and looks like I won't any time soon) to stick in a debugger to have a look at which calls fail exactly causing the status to come back non-zero. Maybe someone else can pick this up?

Comment 19

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

Attachment: Part 1: Pass aFlags through VectorImage::Draw; FLAG_CLAMP is especially important to avoid taking a deathly-slow tiling path

There's quite a lot to work on here. This patch is a huge win, but still leaves me at 109s on this machine where IE10 is about 7s.

The rest of the profile is 30% reflowing SVG because we keep changing the viewport size of the SVG image, and the rest is mostly display list processing while drawing the SVG image. Actual drawing is only about 5%.

Comment 20

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2050e90eb6f

Comment 21

[Ed Morley [:emorley]]

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

Comment 22

[henryfhchan]

is this going to be fixed?

Comment 23

[Mark Straver]

Robert's patch is a big win, and at the very least removes the big regression problem from the bug I mentioned earlier. Firefox 24 beta for me clocks in at around 36 seconds now with surf->CairoStatus() in place.

Comment 24

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

I'd like to work on this some more but haven't had the time so far.

Comment 25

[Mayank Bansal]

On latest nightly : 6.7s
Chrome: 11s

I am calling it fixed.

Comment 26

[Kevin Brosnan [Ex-Mozilla]]

We don't know what fixed this -> works for me