Closed Bug 1475010 Opened 6 years ago Closed 6 years ago

SharedSurfaceANGLE not getting a KeyedMutex

Categories

(Core :: Graphics: CanvasWebGL, defect)

Unspecified
Windows
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- fixed
firefox61 --- wontfix
firefox62 + fixed
firefox63 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

(Keywords: regression, Whiteboard: gfx-noted)

Attachments

(2 files)

This causes us to glFinish every frame.
Whiteboard: gfx-noted
Oops, we dropped ANGLE_ENABLE_KEYEDMUTEX at some point.

I'll make sure we catch this in the future too.
Without KeyedMutex we use glFinish, which is bad.
We accidentally stopped asking ANGLE for KeyedMutexes during some build changes.
See Also: → 1474595
Regression from bug 1440849, I believe.
Blocks: angle-60
Comment on attachment 8991494 [details]
Build ANGLE with ANGLE_ENABLE_KEYEDMUTEX.

Dzmitry Malyshau [:kvark] has approved the revision.

https://phabricator.services.mozilla.com/D2084
Attachment #8991494 - Flags: review+
Comment on attachment 8991494 [details]
Build ANGLE with ANGLE_ENABLE_KEYEDMUTEX.

Jeff Muizelaar [:jrmuizel] has approved the revision.

https://phabricator.services.mozilla.com/D2084
Attachment #8991494 - Flags: review+
checkin-needed?
Thanks, I triggered landing. It should land once trees reopen.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e2c3f2fb8a0
Build ANGLE with ANGLE_ENABLE_KEYEDMUTEX. r=jrmuizel,kvark
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> Thanks, I triggered landing. It should land once trees reopen.

Yo, that's not the final patch I wanted to land. Please let me land it myself in the future.
Flags: needinfo?(bugmail)
Sorry. You weren't around on IRC and also were non-responsive to my previous email on this topic. Want me to back it out?
Flags: needinfo?(bugmail)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> Sorry. You weren't around on IRC and also were non-responsive to my previous
> email on this topic. Want me to back it out?

I can diff it, but I don't see an email from you.
https://hg.mozilla.org/mozilla-central/rev/5e2c3f2fb8a0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 8993865 [details]
Bug 1475010 - Also, assert in DEBUG if we don't get a KeyedMutex. - r=kvark

Dzmitry Malyshau [:kvark] has approved the revision.

https://phabricator.services.mozilla.com/D2276
Attachment #8993865 - Flags: review+
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dade23c56875
Also, assert in DEBUG if we don't get a KeyedMutex. - r=kvark
Is there a user impact here which justifies backport consideration, or can this ride the trains?
Flags: needinfo?(jgilbert)
Yes. This has a noticeable impact on WebGL performance.
Bug 1461336 is a case where a web developer was seeing performance problems with games in Firefox and not Chrome that was said to be helped by this (though I don't think anybody has verified that it helps yet).
In that case, sounds like Beta and ESR60 approval requests are in order :)
Depends on: 1477916
Emailing jgilbert with a reminder.
Comment on attachment 8991494 [details]
Build ANGLE with ANGLE_ENABLE_KEYEDMUTEX.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Simple low-risk perf-regression fix.
User impact if declined: Degraded WebGL perf on Windows.
Fix Landed on Version: 63
Risk to taking this patch (and alternatives if risky): Low, especially after baking on Nightly63. 
String or UUID changes made by this patch: none

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

Approval Request Comment
[Feature/Bug causing the regression]: bug 1440849 (angle-60)
[User impact if declined]: Degraded WebGL perf on Windows.
[Is this code covered by automated tests?]: Test included.
[Has the fix been verified in Nightly?]: Test passes on CI.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: beta62, esr60
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Low risk because it's a flag we always used prior to the ANGLE update in 60, so it's not likely to have broken even for the differing demographics in Beta/ESR.
[String changes made/needed]: none
Flags: needinfo?(jgilbert)
Attachment #8991494 - Flags: approval-mozilla-esr60?
Attachment #8991494 - Flags: approval-mozilla-beta?
Comment on attachment 8991494 [details]
Build ANGLE with ANGLE_ENABLE_KEYEDMUTEX.

Restores us to the same behavior we had prior to the ANGLE-60 update and wins us back some lost performance. Approved for 62.0b17 and ESR 60.2.
Attachment #8991494 - Flags: approval-mozilla-esr60?
Attachment #8991494 - Flags: approval-mozilla-esr60+
Attachment #8991494 - Flags: approval-mozilla-beta?
Attachment #8991494 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: