Closed Bug 814828 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Canvas: 2D, defect)

17 Branch
x86
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla20
Tracking Status
firefox17 --- wontfix
firefox18 + verified
firefox19 + verified
firefox20 + verified
firefox-esr17 - wontfix

People

(Reporter: mahks1, Assigned: nrc)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files, 6 obsolete files)

Attached file fillrect_issue.html (obsolete) —
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
Summary: canvas fillrect 10X slower in FF17 → canvas fillrect 64X slower in FF17
Attached file A better example (obsolete) —
Attachment #684835 - Attachment is obsolete: true
Component: Untriaged → Canvas: 2D
Product: Firefox → Core
Attachment #684836 - Attachment mime type: text/plain → text/html
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).
Attached file 814828 Test case (obsolete) —
Replaced console.time() with alert()
Attachment #684836 - Attachment is obsolete: true
Attached file 812828 test case (obsolete) —
Attachment #684887 - Attachment is obsolete: true
Attached file 814828 test case (obsolete) —
Attachment #684888 - Attachment is obsolete: true
Attached file 814828 test case
Attachment #684889 - Attachment is obsolete: true
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)
(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...
Attachment #684890 - Attachment mime type: text/plain → text/html
(In reply to XtC4UaLL [:xtc4uall] from comment #8)
> only when disabling HWA...

What is HWA?
GPU Hardware Acceleration.
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!
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.
(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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: canvas fillrect 64X slower in FF17 → canvas fillrect 64X slower in Firefox 17 with hardware acceleration disabled
Sorry, read:
bad=2012-08-04
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
Blocks: 773460
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.
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).
(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...
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.
Assignee: nobody → ncameron
(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?
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?
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.
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.
Summary: canvas fillrect 64X slower in Firefox 17 with hardware acceleration disabled → canvas fillRect with op=destOut 64X slower in Firefox 17 with hardware acceleration disabled
(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.
Attached patch patch (obsolete) — Splinter Review
Attachment #685459 - Flags: review?(bas)
Comment on attachment 685459 [details] [diff] [review]
patch

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

This is true.
Attachment #685459 - Flags: review?(bas) → review+
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?
Attachment #685459 - Flags: review+ → review-
Attached patch pacthSplinter Review
Attachment #685459 - Attachment is obsolete: true
Attachment #687653 - Flags: review?(bas)
Attachment #687653 - Flags: review?(bas) → review+
https://hg.mozilla.org/mozilla-central/rev/e5e6d25fdc57
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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
Attachment #687653 - Flags: approval-mozilla-beta?
Attachment #687653 - Flags: approval-mozilla-aurora?
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.
Attachment #687653 - Flags: approval-mozilla-beta?
Attachment #687653 - Flags: approval-mozilla-beta+
Attachment #687653 - Flags: approval-mozilla-aurora?
Attachment #687653 - Flags: approval-mozilla-aurora+
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.
Attachment #687653 - Flags: approval-mozilla-esr17?
Comment on attachment 687653 [details] [diff] [review]
pacth

Low risk canvas perf fix, we can take this for our next ESR.
Attachment #687653 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
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
(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.
(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!
FF 16 <10ms
FF 17 >1000 ms
FF 18b4 <10ms
Verified fixed.
FF 19b3 < 10ms.
Verified fixed Win 7, Ubuntu 12.04, Mac OS X 10.8.2
FF 20b7 < 10ms.
Verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.