1237769 - Disable e10s for XP/D3D9

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

This seems to only happen on XP, with E10S and D3D9 layers, and so far it was only tested on Intel 0x2e32, driver 6.14.10.5420, so not sure how far it goes.  Scrolling (mouse wheel or scroll bar) on http://facebook.com/barackobama creates some nasty effects.  Will attach a screen capture.

Regression range points to us enabling E10S, but that was masked, because we were blocking all acceleration on XP between 36 and 41.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Scrolling with XP+E10S+D3D9

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Works fine when you force D3D9 on Windows 7, with E10S.  Works fine on XP+D3D9 without E10S.  Works fine on XP+E10S and basic compositor.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Anthony has some information from Betabreakers on this.  It could be Intel specific.

Comment 4

[u279076]

(In reply to Milan Sreckovic [:milan] from comment #3)
> Anthony has some information from Betabreakers on this.  It could be Intel
> specific.

I had Betabreakers do some testing to see the breadth of systems affected by this issue. The short version is that *none* of the systems they tested which reported "Direct3D 9 (OMTC)" in about:support exhibited this behaviour. Unfortunately all of the systems which qualified for testing (ie. D3D9 OMTC) had NVIDIA cards, none of their systems with Intel and AMD GPUs met the criteria for testing.

The systems tested were as follows:
* Dell Dimension 9200 desktop w/NVIDIA GeForce 7300LE 256MB GPU w/driver 6.14.13.783 on Windows XP SP3
* Intel Pentium 4 desktop w/NVIDIA GeForce 6600GT 128MB GPU w/driver 6.14.13.783 on Windows XP SP3
* AMD Athlon64 3000+ desktop w/NVIDIA GeForce 7900GS 256MB GPU w/driver 6.14.12.6099 on Windows XP SP3

They tested was as follows:
1) Install Firefox 45.0a2: http://archive.mozilla.org/pub/firefox/nightly/2016/01/2016-01-07-00-40-04-mozilla-aurora/firefox-45.0a2.en-US.win32.installer.exe
2) Make sure "Enable multi-process" is checked (restart Firefox if necessary)		
3) Make sure 'GPU Accelerated Windows' shows as 'Direct3D 9 (OMTC)' in about:support
4) Load facebook.com/barackobama		
6) Scroll around the page for several seconds looking for a rendering issue

As mentioned above, they did not once encounter a rendering issue. If we want broader testing I suppose we need to look internally to see what we can get our hands on and, failing that, reach out to the community for assistance.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 45+ as it sounds like a bad issue on a popular site. 
Maybe we will hear reports of this from e10s experiments on beta for 44 or 45.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Agreed.  Anthony is reaching out to the community to see if we can get testing on more of the targeted systems. (Accelerated XP.)

Comment 7

[u279076]

Attachment: CornelIonce_AboutSupport.txt

Softvision *may* have reproduced this issue or something similar on a Windows XP system with an NVIDIA GeForce GT 620 using driver 6.14.13.3165: http://i.imgur.com/NOshhJd.png

Comment 8

[u279076]

CCing Cornel to provide more information if needed.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Possibly the same problem, I've seen both "smeared" as well as "nothing drawing" scenarios.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

e10s experiment is over and we won't ship e10s in 45, wontfix then.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Looks like D3D9 with e10s is disabled currently, see bug 1237770.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Without fixing this, users who would have had D3D9 will see a noticeable video performance regression

Comment 13

[Jeff Muizelaar [:jrmuizel]]

We have approx 5% of users on D3D9. I suspect some chunk of them are on XP.

Comment 14

[George Wright (:gw280) (needinfo me!)]

I just tested this and it's an issue on nightly currently, and is *not* APZC related.

Comment 15

[George Wright (:gw280) (needinfo me!)]

This is potentially related to bug 1169331

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

What we really want, in absence of a fix, is to disable E10S when we're using D3D9.  I have a feeling the timing for making those decisions is not helping us, and that at the time that we're deciding E10S-or-not, we do not yet have the information whether we're using D3D9 or not.

George, re: comment 14, did you test on an XP system, or did you just force the D3D9 use?  Or, more to the point, what's the system you tested on?

Comment 17

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

e10s on xp with any hw accel sounds terrifying.  I would suggest treating XP as basic compositor only, perhaps with some compositor-side-only fast paths (e.g. for video) -- nothing cross process.

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

Yes, if this ends up being XP only, rather than D3D9 issue, I'd say we disable E10S on XP to start.  We then figure out (if we want to and can do) the suggestion from comment 17.

Comment 19

[Jim Mathies [:jimm]]

(In reply to Milan Sreckovic [:milan] from comment #18)
> Yes, if this ends up being XP only, rather than D3D9 issue, I'd say we
> disable E10S on XP to start.  We then figure out (if we want to and can do)
> the suggestion from comment 17.

Would this be permanent? If so it could force us to maintain non-e10s automation test coverage for XP. We have three existing exclusions (a11y, ltr, addons) but fixes are in the works there. The goal is to turn off non-e10s test coverage as soon possible.

Comment 20

[George Wright (:gw280) (needinfo me!)]

(In reply to Milan Sreckovic [:milan] from comment #16)
> What we really want, in absence of a fix, is to disable E10S when we're
> using D3D9.  I have a feeling the timing for making those decisions is not
> helping us, and that at the time that we're deciding E10S-or-not, we do not
> yet have the information whether we're using D3D9 or not.
> 
> George, re: comment 14, did you test on an XP system, or did you just force
> the D3D9 use?  Or, more to the point, what's the system you tested on?

I tested on the XP system in the office, the desktop behind us.

Comment 21

[George Wright (:gw280) (needinfo me!)]

So the status quo is that if we detect e10s is on, we disable d3d9 and only allow basic. This is on XP only, according to the patch at https://bugzilla.mozilla.org/show_bug.cgi?id=1237770

Going forward, it looks like the options available to us are:

- Keep it the same as it currently is. This would involve shipping a regression to our users on XP when e10s rolls out, as they would go from non-e10s w/d3d9 -> e10s w/basic.
- Invert the workaround by detecting if we have d3d9/xp, and disabling e10s rollout to those people. This involves shipping them exactly what they currently have, non-e10s w/d3d9. This is the recommended solution I believe, but it isn't a long-term solution.
- Long term, evaluate whether we want to support d3d9 with e10s (based on vlad's comment 17) and if we don't, then work out what needs to be done to make the performance acceptable. If we do, then at a minimum fix the streaky scrolling issue.

On a side note: afaik we have only ever reproduced this bug on a specific XP machine in the Toronto office. As a result, I don't think we have an accurate image of how much impact this particular bug has, especially given comment 3 stating that none of the machines QA has can reproduce this issue.

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to George Wright (:gw280) (:gwright) from comment #21)
> - Invert the workaround by detecting if we have d3d9/xp, and disabling e10s
> rollout to those people. This involves shipping them exactly what they
> currently have, non-e10s w/d3d9. This is the recommended solution I believe,
> but it isn't a long-term solution.

This would be even more restrictive - no E10S on XP.  The way we initialize things, the decision whether we can use D3D9 or not is done too late for the E10S decision to depend on it.

And, yes, agreed, this is not a long term solution, but at least we have the interim without user level regressions - they didn't have E10S before, they wouldn't have E10S after this change.

(Technically, we could disable E10S on XP that *requests* acceleration, rather than *gets* acceleration, which would mean that setting a preference to disable acceleration would enable E10S on XP.)

Comment 23

[Milan Sreckovic [:milan] (needinfo for best results)]

George, can you put together a patch for "disable E10S under Windows XP when acceleration is requested" and we can use that in the further conversations?  Not sure if we'd put it under this, or under bug 1237776.

Comment 24

[Jim Mathies [:jimm]]

(In reply to Milan Sreckovic [:milan] from comment #23)
> George, can you put together a patch for "disable E10S under Windows XP when
> acceleration is requested" and we can use that in the further conversations?
> Not sure if we'd put it under this, or under bug 1237776.

How long do you think this switch will be in place?

Comment 25

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Milan: can we compare performance of two configurations of Firefox:

A) d3d9 is disabled, e10s is enabled
B) d3d9 is enabled, e10s is disabled


I want to know what performance trade-offs there are, if any.

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Jim Mathies [:jimm] from comment #24)
> (In reply to Milan Sreckovic [:milan] from comment #23)
> > George, can you put together a patch for "disable E10S under Windows XP when
> > acceleration is requested" and we can use that in the further conversations?
> > Not sure if we'd put it under this, or under bug 1237776.
> 
> How long do you think this switch will be in place?

Good question.

We want the user experience with E10S to deteriorate for the XP+acceleration users.  Once we can demonstrate that there are no regressions from XP+acceleration to E10S+XP+no acceleration, we can remove this switch.  Today, we have that regression in place by forcing acceleration off when E10S is on.  So, good for us (we don't have to worry about supporting E10S and non-E10S), bad for the user (they experience performance regressions.)

What does this mean in practice?  We could consider it blocked on meta bug 1253062, but we can probably get it through once we're "close".

Comment 27

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #25)
> Milan: can we compare performance of two configurations of Firefox:
> 
> A) d3d9 is disabled, e10s is enabled
> B) d3d9 is enabled, e10s is disabled
> 
> 
> I want to know what performance trade-offs there are, if any.

That can certainly be done.  Who can do that?

Comment 28

[George Wright (:gw280) (needinfo me!)]

(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #25)
> Milan: can we compare performance of two configurations of Firefox:
> 
> A) d3d9 is disabled, e10s is enabled
> B) d3d9 is enabled, e10s is disabled
> 
> 
> I want to know what performance trade-offs there are, if any.

Are there any metrics in particular you're interested in?

Comment 29

[Jim Mathies [:jimm]]

Can we run this through try? I'm not sure if we have the ability to measure d2d9 on/off under xp there. That's the first question to answer. If not it's going to have to be manual testing I guess which is pretty worthless.

In the interim, the gfx team wants to flip d2d9 back on for xp and disable e10s. Can you put together a patch for that so we have it ready to land?

Comment 30

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-1237769-Disable-e10s-on-Windows-XP-if-layers-acc.patch

Disable e10s on WinXP when layers.acceleration.disabled is false or if layers.acceleration.force-enabled is true.

Comment 31

[:Felipe Gomes (needinfo for replies!)]

Two notes:

The #ifdef block that you added this code has been changed to not be Windows only on bug 1260190.

And see the note above the enum in nsAppRunner.cpp. You'll need to update the about:support code to add a string with an explanation for this disabling (in aboutSupport.properties and it needs to be whitelisted in aboutSupport.js).

Since this will require a new string it will need a non-string version to be uplifted to 47. I've given some thought to it before. Ping me on IRC or i'll post details later.

Comment 32

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8738261 [details] [diff] [review]
0001-Bug-1237769-Disable-e10s-on-Windows-XP-if-layers-acc.patch

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

::: gfx/thebes/gfxPlatform.cpp
@@ +2066,5 @@
>        sLayersSupportsD3D11 = true;
>      } else if (gfxInfo) {
>        if (NS_SUCCEEDED(gfxInfo->GetFeatureStatus(nsIGfxInfo::FEATURE_DIRECT3D_9_LAYERS, &status))) {
>          if (status == nsIGfxInfo::FEATURE_STATUS_OK) {
> +          sLayersSupportsD3D9 = true;

Let's be paranoid and add:

MOZ_ASSERT(!sPrefBrowserTabsRemoteAutostart || IsVistaOrLater());

::: toolkit/xre/nsAppRunner.cpp
@@ +4660,5 @@
>    // kE10sDisabledForMacGfx = 5, was removed in bug 1068674.
>    kE10sDisabledForBidi = 6,
>    kE10sDisabledForAddons = 7,
>    kE10sForceDisabled = 8,
> +  kE10sDisabledForXPAcceleration = 9,

We're not interpreting these enums somewhere else, with the magically matching numbers, right?

Comment 33

[George Wright (:gw280) (needinfo me!)]

(In reply to Milan Sreckovic [:milan] from comment #32)
> ::: toolkit/xre/nsAppRunner.cpp
> @@ +4660,5 @@
> >    // kE10sDisabledForMacGfx = 5, was removed in bug 1068674.
> >    kE10sDisabledForBidi = 6,
> >    kE10sDisabledForAddons = 7,
> >    kE10sForceDisabled = 8,
> > +  kE10sDisabledForXPAcceleration = 9,
> 
> We're not interpreting these enums somewhere else, with the magically
> matching numbers, right?

Yep, I need to add a string corresponding to this to explain why e10s is blocked. As I understand it, the string change is not an issue as we're letting this ride the trains and not uplifting.

Comment 34

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Bug-1237769-Disable-e10s-on-Windows-XP-if-layers-acc.patch

Comment 35

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8738283 [details] [diff] [review]
0001-Bug-1237769-Disable-e10s-on-Windows-XP-if-layers-acc.patch

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

::: toolkit/locales/en-US/chrome/global/aboutSupport.properties
@@ +107,5 @@
>  multiProcessStatus.5 = Disabled by lack of graphics hardware acceleration
>  multiProcessStatus.6 = Disabled by unsupported text input
>  multiProcessStatus.7 = Disabled by add-ons
> +multiProcessStatus.8 = Disabled forcibly
> +multiProcessStatus.9 = Disabled by accelerated layers on Windows XP

Nit: I understand #5 has been made obsolete, but perhaps it's worth having a consistent wording like
Disabled by graphics hardware acceleration on Windows XP
to match the other one.  Where the other one probably should have had "on Mac OS X" :)
As I said, nit.

Comment 36

[Pulsebot]

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

Comment 37

[Carsten Book [:Tomcat]]

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

Comment 38

[Ton [:Tonnes]]

(In reply to Milan Sreckovic [:milan] from comment #35)
> Review of attachment 8738283 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/locales/en-US/chrome/global/aboutSupport.properties
> @@ +107,5 @@
> >  multiProcessStatus.5 = Disabled by lack of graphics hardware acceleration
> >  multiProcessStatus.6 = Disabled by unsupported text input
> >  multiProcessStatus.7 = Disabled by add-ons
> > +multiProcessStatus.8 = Disabled forcibly
> > +multiProcessStatus.9 = Disabled by accelerated layers on Windows XP
> 
> Nit: I understand #5 has been made obsolete, but perhaps it's worth having a
> consistent wording like
> Disabled by graphics hardware acceleration on Windows XP
> to match the other one.  Where the other one probably should have had "on
> Mac OS X" :)

> https://hg.mozilla.org/mozilla-central/rev/a3f8a4e83cdd
> +multiProcessStatus.5 = Disabled by lack of graphics hardware acceleration on Mac OS X
> +multiProcessStatus.9 = Disabled by graphics hardware acceleration on Windows XP

Shouldn’t .9 use "Disabled by *lack of* graphics hardware acceleration on Windows XP" for that same reason?

Comment 39

[Théo Chevalier]

Please note that the change made to multiProcessStatus.5 won’t be picked by localizers unless you change the string ID.

Comment 40

[Ton [:Tonnes]]

Agree, noticed it because of using diffs instead of l10n dashboard.

If adding "lack of" is OK, I’m also confused about whether .9 should read
Disabled by lack of accelerated layers on Windows XP
or
Disabled by lack of graphics hardware acceleration on Windows XP
...though both seem to have the same meaning.

Comment 41

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Ton from comment #40)
> Agree, noticed it because of using diffs instead of l10n dashboard.
> 
> If adding "lack of" is OK, I’m also confused about whether .9 should read
> Disabled by lack of accelerated layers on Windows XP
> or
> Disabled by lack of graphics hardware acceleration on Windows XP
> ...though both seem to have the same meaning.

It should be the way it is, without the "lack of" for .9 - when we detect acceleration on Windows XP, we go ahead and disable E10S.  Accelerated XP and E10S are currently incompatible.