Closed Bug 1260507 Opened 8 years ago Closed 8 years ago

Disable D3D11 WARP in favour of Basic

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s m9+ ---
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- fixed

People

(Reporter: gw280, Assigned: gw280)

References

Details

(Keywords: perf)

Attachments

(1 file)

Follow up to bug 1213432 and bug 1254877. Test data shows that WARP performs slower than Basic at the moment, and the delta gets larger with e10s.

We should prefer Basic layers until we can figure out why WARP is slower. In its current state, because the delta is larger in e10s, shipping e10s will likely block on this.
Attachment #8736000 - Flags: review?(bas)
For video, we only have the data from one computer, and with E10S on (bug 1254877), telling us that basic is good enough.  And we have a bug for improving unaccelerated video performance (meta bug 1253062) with a number of open blockers.

1. We could proceed with this patch on nightly, see what happens.  I would not want to uplift it, and if we do, I think we'd want to do an "experiment" on Beta to see what happens.
2. We could have a "disable WARP by default if E10S is on" solution. That has a downside of the users not being able to force WARP back on, but is probably something we could uplift easier than option #1.
3. We could block this on the bug 1253062, not disabling WARP until we're happy with the basic compositor video performance.

I'd probably go with #1 as a balance of simplicity and prudence.  Don't know that I'd push hard for the experiment.
Attachment #8736000 - Flags: review?(bas) → review+
Going with option 1 then. Landing this on Nightly and let's keep an eye on things.
I have patches to improve WARP performance, they will probably positively impact D3D11 performance in general. Those might be a better solution.
Flags: needinfo?(gwright)
Assignee: nobody → gwright
Flags: needinfo?(gwright)
OK, let's see what the performance delta is once your patches are in and re-assess.
https://hg.mozilla.org/mozilla-central/rev/25ff944d5dc9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
See Also: → 1254877
George, should this fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47?
Flags: needinfo?(gwright)
I think so.
Flags: needinfo?(gwright)
We are planning on running "external" tests (betabreakers and community) on this change - but that will only happen once 48 is on Aurora.  I'm somewhat reluctant to just unleash this on the unsuspecting Beta 47 population without knowing a little bit more.

What's the timing for the Beta 47 tests?  Anthony, what's our potential timing for betabreakers Aurora 48 tests?
Flags: needinfo?(cpeterson)
Flags: needinfo?(anthony.s.hughes)
(In reply to Milan Sreckovic [:milan] from comment #10)
> What's the timing for the Beta 47 tests?  Anthony, what's our potential
> timing for betabreakers Aurora 48 tests?

We plan to run a continuous e10s test with 50% of the Beta population starting in Beta 47. If this is a risky change, then we don't need to rush it to Aurora 47.
Flags: needinfo?(cpeterson)
I don't think it's risky as such, just that we haven't collected the data on it.  Anthony will give us the data, we can uplift this now, and be ready to back it out of Beta if either the Betabreakers testing, or user feedback tells us something is wrong.
(In reply to Milan Sreckovic [:milan] from comment #10)
> What's the timing for the Beta 47 tests? What's our potential timing for betabreakers Aurora 48 tests?

I am not prepared to give you the data you need to assess this uplift in time. As requested earlier Betabreakers testruns are intended to vet disabling WARP in Aurora 48 and D3D9 scrolling performance (bug 1253062) in Beta 47. As usual, these testruns would start a couple weeks in to the cycle and wrap up mid-way through the cycle.

We have 12 working days before the next merge so I could try to move up the timeline, doing WARP testing in Nightly 48 or Aurora 47 if it's uplifted. However, I'm still working on the test plan for this and have not yet been informed of Betabreakers next availability. Moving this up increases the likelihood of something getting missed and carries no guarantee that you'll get the information you need before this moves to Beta.

There's also no guarantee that Betabreakers and our Beta users will identify issues if this is uplifted. This feels like one of those things that could cause issues we're blind to until it hits our much more diverse Release audience. 

In general, I'd rather that we make decisions based on supporting data, not revert decisions based on refuting data. That said, I am no expert on the code, the associated risk, nor how this maps to our top-line goals. I will defer to Milan in this regard.

Please needinfo? me if you'd like me to alter my plans, otherwise I will proceed with my original plan as detailed in my first paragraph above.
Flags: needinfo?(anthony.s.hughes)
Thanks, Anthony. It sounds like this change should ride the trains with 48 as planned.
You need to log in before you can comment on or make changes to this bug.