599698 - very slow in IE9 Speed Reading benchmark [UpdateCurrentClip issue]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

Not sure what this thing's doing, but we're quite a bit slower than IE9 here.  From looking at the code, it looks like just a lot of drawImage().

Comment 1

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

Bas looked into this some, and I just did as well in xperf.

Inside 15.54/24.88 spent in Canvas 2D DrawImage, the biggest chunk is 5.56/24.88 spent in UpdateSurfaceClip.  Do we even need this any more?  If we still need it for GDI, can we not do it for d2d surfaces?

2.55/24.88 is spent inside the actual Paint() call, and then 1.42/24.88 is spent inside DoDrawImageSecurityCheck.  For the latter, I'm kind of surprised it's taking up that much; this benchmark does a lot of drawImage calls, but that's still a lot of overhead.  Then 1.08/24.88 is in SurfaceFromElement, which again seems high for an image surface.  Inside there it's actually mostly in assign_from_qi, which is a little odd.

Looking into whether we need the UpdateSurfaceClip now -- it was added in bug 346421.

Comment 2

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

In fact, if I remove the UpdateSurfaceClip, we easily hit 60 fps.

Comment 3

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

Attachment: let's get rid of this

So this was added in 2007 to work around a pixman bug.  I can't reproduce the original bug with the testcase in the old bug with or without d2d, which leads me to believe that the pixman bug is fixed (it was filed upstream, and I know clipping has been reworked at least twice since then).  So I think we should just get rid of this, because 60fps is a lot more fps than 28!

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 478621 [details] [diff] [review]
let's get rid of this

r+a+omfgloli<3u=shaver

Comment 5

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

Actually -- today's nightly (now that I updated from a few days ago) was about as fast, also 60fps at the start.  So this isn't going to help a huge amount, but it helps.

http://hg.mozilla.org/mozilla-central/rev/2370ecfdcbb7

Comment 6

[Csaba Kozák [:WonderCsabo]]

I just tried latest hourly with this patch.

My resutls:

Fx4: 
Average FPS: 40 FPS
Average Drawing time: 26 ms
Browser score: 679 seconds

IE9:
Average FPS: 60 FPS
Average Drawing time: 16 ms
Browser score: 19 seconds

Something is bad here. I have D2D,DW,layers,JM+TM ON.

Comment 7

[Myzar]

Same here it's still very slow compared to Ie9 , i've 60fps before the benchmark starts then it drops to 25/30 and it seems to slowdown further with the bench running for awhile

the score is 10sec vs 68

Comment 8

[Bas Schouten (:bas.schouten)]

Wait until D3D10 layers land, that will bring it up another bunch!

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

s: talos-r3-fed64-007
29447 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.composite.uncovered.image.destination-atop.html | pixel 15,15 is 0,255,255,128; expected 0,0,0,0 +/- 5
29448 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.composite.uncovered.image.destination-atop.html | pixel 50,25 is 0,255,255,128; expected 0,0,0,0 +/- 5
29451 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.composite.uncovered.image.destination-in.html | pixel 15,15 is 0,255,255,128; expected 0,0,0,0 +/- 5
29452 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.composite.uncovered.image.destination-in.html | pixel 50,25 is 0,255,255,128; expected 0,0,0,0 +/- 5
29455 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.composite.uncovered.image.source-in.html | pixel 15,15 is 0,255,255,128; expected 0,0,0,0 +/- 5
29456 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.composite.uncovered.image.source-in.html | pixel 50,25 is 0,255,255,128; expected 0,0,0,0 +/- 5
29459 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.composite.uncovered.image.source-out.html | pixel 15,15 is 0,255,255,128; expected 0,0,0,0 +/- 5
29460 ERROR TEST-UNEXPECTED-FAIL | /tests/content/canvas/test/test_2d.composite.uncovered.image.source-out.html | pixel 50,25 is 0,255,255,128; expected 0,0,0,0 +/- 5

http://hg.mozilla.org/mozilla-central/rev/49cc66b9f097

Comment 10

[Bas Schouten (:bas.schouten)]

We can probably just #ifndef linux this if the pixman bug still exists for linux, but let's see if the rest of the tests comes back clean.

Comment 11

[José Jeria]

Build ID: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100926 Firefox/4.0b7pre

Don't know if this patch made this change, but I get 21 seconds now on the IE speed reading demo with today's build compared to 54 seconds with yesterdays (20100925) build.

Comment 12

[Bas Schouten (:bas.schouten)]

Performance has definitely improved for me on the test. However I too see the slow rising of per-frame drawing times. Starting out at +/- 10 ms and slowly climbing to 20-25 during the test.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

These failures occurred on every platform.

Comment 14

[Bas Schouten (:bas.schouten)]

So, I suspect this is because with non-OPERATOR_OVER we now no longer blow away the path. All these failing test cases are not using operator over. A simple context->NewPath() should probably fix this I'm guessing.

Comment 15

[Gary [:streetwolf52]]

Please explain the phenomena I see with this test, and many others, that when I run a test then restart Fx4 and run it again my scores are better?  

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100924 Firefox/4.0b7pre - Build ID: 20100926061048

Comment 16

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

(In reply to comment #12)
> Performance has definitely improved for me on the test. However I too see the
> slow rising of per-frame drawing times. Starting out at +/- 10 ms and slowly
> climbing to 20-25 during the test.

Yep, it does that in all browsers; no idea why.

(In reply to comment #14)
> So, I suspect this is because with non-OPERATOR_OVER we now no longer blow away
> the path. All these failing test cases are not using operator over. A simple
> context->NewPath() should probably fix this I'm guessing.

hmm.. we have a gfxContextPathAutoSaveRestore here that should be recovering the path.  I'll look more.

For those who are seeing worse results in Fx4, what graphics hardware are you on?  We established last night that this is hardware dependent.. I can match IE's score on my nvidia laptop, but shaver was unable to on his intel laptop.

Comment 17

[Csaba Kozák [:WonderCsabo]]

I'm on a HD4330 with 512MB DDR2 RAM.

Comment 18

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

Actually the test problem was just revealed by this patch, instead of being caused by it.  With non-OVER the call to fill inside UpdateSurfaceClip, even with a 0-sized rectangle, was causing rendering to happen outside of the rectangle -- specifically, to clear the remainder of the clip area.  So somewhere we're not handling these operators correctly...

Comment 19

[Myzar]

i'm on a 8800gt 512mb ram with drivers 260.63

Comment 20

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

In fact, the behaviour with the patch is correct, according to philip's upstream tests -- see

  http://philip.html5.org/tests/canvas/suite/tests/2d.composite.image.destination-in.html
  http://philip.html5.org/tests/canvas/suite/tests/2d.composite.image.destination-atop.html

etc.

Looks like we need to update our tests...

Comment 21

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

(no, I take that back, I was looking at the wrong tests -- but those tests pass with my patch, where they failed before)

Comment 22

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

interestingly, Chrome fails the 'uncovered' tests, but passes the others.  and in our Makefiles, we have the non-uncovered tests disabled.

Comment 23

[Masatoshi Kimura [:emk]]

Does OperatorAffectsUncoveredAreas function need to be restored? (which was removed by bug 366283). See also bug 503283.

Comment 24

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

Attachment: same patch, but with test updates

This is the identical code path, but updates tests.  I've filed bug 600120 for the composite.uncovered.image tests which likely shouldn't have been passing in the first place.

This has been through the try server, even!

Comment 25

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

Oh, I should mention -- this patch takes speed reading from ~20 fps -> ~45 fps on my intel GMA HD laptop; it's not a complete fix for the benchmark, but it certainly helps.

Comment 26

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

http://hg.mozilla.org/mozilla-central/rev/98559eb0a783

So I'm going to call this bug resolved, for the UpdateSurfaceClip call; the remaining perf issues should be covered by bug 600410.

Comment 27

[Tomas]

Perhaps b7 blocker?

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This will be fixed in b7.