814828 - canvas fillRect with op=destOut 64X slower in Firefox 17 with hardware acceleration disabled

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Mahks]

Attachment: fillrect_issue.html

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121024073032

Steps to reproduce:

canvas.fillRect() taking 64 times longer (or more) in FF17 than in FF16
Try the test case attached in both versions.
Check your console for the draw times


Actual results:

Tried to run same code in FF17


Expected results:

Same render time as FF16

Comment 1

[Mahks]

Attachment: A better example

Comment 2

[Loic]

I am not able to reproduce the slowness on Win 7. With FF16 or FF17, the canvas takes the same time to render (~430 ms after 10 runs).

Comment 3

[Mahks]

Attachment: 814828 Test case

Replaced console.time() with alert()

Comment 4

[Mahks]

Attachment: 812828 test case

Comment 5

[Mahks]

Attachment: 814828 test case

Comment 6

[Mahks]

Attachment: 814828 test case

Comment 7

[Mahks]

Sorry, Does not let me save the attachment in other than plain.

Am testing on two separate systems, both Win7, no FF extensions.

I am getting 10~20ms on FF16 

FF17: 1200~1300ms and 2000~2050ms (slower CPU)

Comment 8

[(mostly gone) XtC4UaLL [:xtc4uall]]

(In reply to Loic from comment #2)
> I am not able to reproduce the slowness on Win 7. With FF16 or FF17, the
> canvas takes the same time to render (~430 ms after 10 runs).

Same over here. I get slower Results (Factor 8-9) only when disabling HWA...

Comment 9

[Mahks]

(In reply to XtC4UaLL [:xtc4uall] from comment #8)
> only when disabling HWA...

What is HWA?

Comment 10

[(mostly gone) XtC4UaLL [:xtc4uall]]

GPU Hardware Acceleration.

Comment 11

[Mahks]

I am tracking the draw times for the users of my web site. All are getting the slowness except one, he is still on FF16!

Comment 12

[Mahks]

I've sent the above attachment out to several users.
They are reporting back with the same discrepancy in times between those on FF16 vs FF17.

Comment 13

[Loic]

(In reply to XtC4UaLL [:xtc4uall] from comment #8)
> (In reply to Loic from comment #2)
> > I am not able to reproduce the slowness on Win 7. With FF16 or FF17, the
> > canvas takes the same time to render (~430 ms after 10 runs).
> 
> Same over here. I get slower Results (Factor 8-9) only when disabling HWA...

Confirmed, only with HWA off.
With HWA on: ~1600-1700 ms
With HWA off: ~23 ms

m-c
good=2012-08-03
bad=2012-08-03
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=89dcadd42ec4&tochange=20fc34efd733

My guess goes to:
Nicholas Cameron — Bug 773460. Pref on Azure/Cairo for Windows. r=roc

Comment 14

[Loic]

Sorry, read:
bad=2012-08-04

Comment 15

[Alice0775 White]

in condition of HWA off,setting  gfx.canvas.azure.backends = direct2d helps

Regression window with HWA off(m-c)
12ms:
http://hg.mozilla.org/mozilla-central/rev/73b3b3f828b0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120803042624
1470ms:
http://hg.mozilla.org/mozilla-central/rev/62d4f0efe485
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120803073024
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=73b3b3f828b0&tochange=62d4f0efe485

Regression window with HWA off(m-i)
11ms:
http://hg.mozilla.org/integration/mozilla-inbound/rev/3a17236e9084
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120802142137
1407ms:
http://hg.mozilla.org/integration/mozilla-inbound/rev/032ba64ab1f1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120802150336
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3a17236e9084&tochange=032ba64ab1f1

Suspected: Bug 773460

Comment 16

[Nick Cameron [:nrc]]

I would guess that Thebes canvas was using the D2D Cairo backend, but Azure Canvas uses some slower backend for Cairo. I'll try to confirm this.

Comment 17

[Nick Cameron [:nrc]]

Confirmed that Azure canvas ends up using win32 surface backend for Cairo. I assume that with Thebes canvas, we end up with the D2D surface backend, but there seems no point in confirming this because it will be very system dependent (I've previously seen this happen).

Comment 18

[Nick Cameron [:nrc]]

(In reply to Nick Cameron [:nrc] from comment #17)
> Confirmed that Azure canvas ends up using win32 surface backend for Cairo. I
> assume that with Thebes canvas, we end up with the D2D surface backend, but
> there seems no point in confirming this because it will be very system
> dependent (I've previously seen this happen).

Or not, actually unlikely that Cairo is using D2D behind our back. I'll try and check what's going on at some point...

Comment 19

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

Since this is a regression, please try to figure it out as soon as possible. If you can reproduce the bug and get a profile of the slow case, that'll probably be enough.

Comment 20

[Nick Cameron [:nrc]]

(In reply to Loic from comment #13)
> (In reply to XtC4UaLL [:xtc4uall] from comment #8)
> > (In reply to Loic from comment #2)
> > > I am not able to reproduce the slowness on Win 7. With FF16 or FF17, the
> > > canvas takes the same time to render (~430 ms after 10 runs).
> > 
> > Same over here. I get slower Results (Factor 8-9) only when disabling HWA...
> 
> Confirmed, only with HWA off.
> With HWA on: ~1600-1700 ms
> With HWA off: ~23 ms
> 
> m-c
> good=2012-08-03
> bad=2012-08-03
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=89dcadd42ec4&tochange=20fc34efd733
> 
> My guess goes to:
> Nicholas Cameron — Bug 773460. Pref on Azure/Cairo for Windows. r=roc

IS that the right way round for HWA on/off?

I get ~50ms with HWA on and ~550ms with HWA and D2D off. With latest nightly, which is kind of expected. With FF16, with HWA on I get 100ms and HWA off I get 5ms. So with HWA we get twice as fast and without we get 110 times slower! Huh, what? And non-accelerated was 20 times faster than accelerated?

Comment 21

[Nick Cameron [:nrc]]

Bas, Jeff: do you know of any Cairo optimisation we might be missing out on? Or is this likely just due to overhead of Azure?

Comment 22

[Nick Cameron [:nrc]]

So, the slowdown is due to the test case using ctx.globalCompositeOperation = 'destination-out', which causes us to take a slow path in DrawTargetCairo::DrawPattern. In theory we shouldn't need to affect uncovered areas with destination out composition, but I think I remember needing to do it for some reason. I'm making a build without this which should be fast, but I'll need to check it is correct.

Comment 23

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

Updating the summary -- if this is due to dest-out and regular over fills are still just as fast/faster, then it's somewhat less urgent.

Comment 24

[Nick Cameron [:nrc]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #23)
> Updating the summary -- if this is due to dest-out and regular over fills
> are still just as fast/faster, then it's somewhat less urgent.

Yep, regular fills are just as fast.

Comment 25

[Nick Cameron [:nrc]]

Looks good on try: https://tbpl.mozilla.org/?tree=Try&rev=c98b927e9fce

Comment 26

[Nick Cameron [:nrc]]

Attachment: patch

Comment 27

[Bas Schouten (:bas.schouten)]

Comment on attachment 685459 [details] [diff] [review]
patch

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

This is true.

Comment 28

[Bas Schouten (:bas.schouten)]

Comment on attachment 685459 [details] [diff] [review]
patch

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

Now that I think about it.. how about we just use the function already in Tools.h?

Comment 29

[Nick Cameron [:nrc]]

new patch: https://tbpl.mozilla.org/?tree=Try&rev=b896791c678a

Comment 30

[Nick Cameron [:nrc]]

Attachment: pacth

Comment 31

[Nick Cameron [:nrc]]

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

Comment 32

[Ed Morley [:emorley]]

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

Comment 33

[Nick Cameron [:nrc]]

Comment on attachment 687653 [details] [diff] [review]
pacth

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: poor performance for some canvas operations
Testing completed (on m-c, etc.): m-c a few days
Risk to taking this patch (and alternatives if risky): low, it's an optimisation only, and previous behaviour was technically incorrect.
String or UUID changes made by this patch: none

Comment 34

[Alex Keybl [:akeybl]]

Comment on attachment 687653 [details] [diff] [review]
pacth

Approving for Aurora/Beta given the magnitude of this canvas perf regression, this fix is considered low risk, and the fact that we're not sure what the volume of affected sites would be.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bad0552d0442
https://hg.mozilla.org/releases/mozilla-beta/rev/23c595cd1ec4

Comment 36

[Nick Cameron [:nrc]]

Comment on attachment 687653 [details] [diff] [review]
pacth

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: optimisation leading to vastly better performance in some cases, addresses regression, simple patch
User impact if declined: slow canvas rendering in some cases 
Fix Landed on Version: 18,19,20
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 37

[Alex Keybl [:akeybl]]

Comment on attachment 687653 [details] [diff] [review]
pacth

Low risk canvas perf fix, we can take this for our next ESR.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/99313a46008c

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Nick, this seems to be causing test failures on ESR17. Can you take a look at what's going on please?
https://tbpl.mozilla.org/php/getParsedLog.php?id=17802670&tree=Mozilla-Esr17

Comment 40

[Nick Cameron [:nrc]]

(In reply to Ryan VanderMeulen from comment #39)
> Nick, this seems to be causing test failures on ESR17. Can you take a look
> at what's going on please?
> https://tbpl.mozilla.org/php/getParsedLog.php?id=17802670&tree=Mozilla-Esr17

Hmm, I can't see what is going wrong, the errors are with the xor operator and we shouldn't touch paths that are xor. I can only assume that something changed between 17 and 18 to make this patch not work properly. Given that this is just a perf issue and is a relatively rare case, I don't think it is worth spending a lot of time investigating. I think we should just back it out and not land on esr17. Would you mind doing that? I don't have an esr17 tree, sorry.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Thanks for looking into it. Backed out.
https://hg.mozilla.org/releases/mozilla-esr17/rev/e3ba5fa73f73

Comment 42

[Nick Cameron [:nrc]]

(In reply to Ryan VanderMeulen from comment #41)
> Thanks for looking into it. Backed out.
> https://hg.mozilla.org/releases/mozilla-esr17/rev/e3ba5fa73f73

Thanks for the backout!

Comment 43

[Paul Silaghi, QA [:pauly]]

FF 16 <10ms
FF 17 >1000 ms
FF 18b4 <10ms
Verified fixed.

Comment 44

[Paul Silaghi, QA [:pauly]]

FF 19b3 < 10ms.
Verified fixed Win 7, Ubuntu 12.04, Mac OS X 10.8.2

Comment 45

[Paul Silaghi, QA [:pauly]]

FF 20b7 < 10ms.
Verified fixed.