[e10s] Scrolling Facebook with D3D9 acceleration is worse than without acceleration

RESOLVED FIXED in Firefox 47

Status

()

Core
Graphics
P5
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: vladan, Assigned: BenWa)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox47 fixed)

Details

(Whiteboard: [content perf][gfx-noted])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
This was discovered during content-performance testing. 

STR:

1. Start Nightly on Windows in e10s mode, with D3D9 acceleration enabled (layers.prefer-d3d9 = true), D2D disabled (gfx.direct2d.disabled = true) and the laptop in “Power Saver” mode (but plugged in)
2. Open https://www.facebook.com/barackobama without being logged into Facebook
3. Scroll top to bottom using the scrolling bookmarklet attached to bug 894128 (it does not trigger APZ btw)
4. Repeat above steps with acceleration disabled: gfx.direct2d.disabled = true and layers.acceleration.disabled = true
5. Compare FPS and consistency of scrolling with & without acceleration

We tested with Nightly e10s on Windows 8.1 on the content-perf test laptop: HP Pavillion 14t i3-5010u in “power saver” mode (plugged into power)

Avi notes that without e10s he saw the opposite result, i.e. D3D9 acceleration was benefitting scrolling

Note that our other 2 references pages (Yahoo and Twitter) were not taxing enough to cause meaningful performance differences in this test setup
(Reporter)

Updated

2 years ago
See Also: → bug 1213432
(Reporter)

Updated

2 years ago
Blocks: 1213469
Bas, are we concerned about d3d9?
Flags: needinfo?(bas)
If this is e10s related I'm guessing this is somehow related to async scrolling? Those peeps would know more. In general, as we see without e10s apparently, we expect scrolling to be significantly better with D3D9. One thing that could be happening is that we're uploading too much with e10s since we'll be using BufferTextureClients.
Flags: needinfo?(bas)
(Reporter)

Comment 3

2 years ago
> If this is e10s related I'm guessing this is somehow related to async scrolling?

It's not, the scrolling in this bug was done with the scrolling bookmarklet (see step 3 above), so APZ wasn't used
Whiteboard: [content perf] → [content perf][gfx-noted]
Is this a regression on XP (which is the only platform where D3D9 is supported)?
Flags: needinfo?(vladan.bugzilla)
(Reporter)

Comment 5

2 years ago
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #4)
> Is this a regression on XP (which is the only platform where D3D9 is
> supported)?

Correct. We don't want to put Windows XP on our reference hardware so it was tested on Windows 8 by forcing D3D9 via prefs.
Flags: needinfo?(vladan.bugzilla)
Windows XP results:

e10s, acceleration
==================
- Histogram has a spike in the 80-120ms bucket
- Visually the scrolling appears to speed up and then slow down cyclically

mean/std dev:
60.14/42.75
36.17/27.92
54.16/39.86
44.55/35.80
52.02/39.34

e10s, no acceleration
=====================
- Visually: smoother, no perceived change in velocity
- Distribution is more normal
- Std dev significantly better

mean/std dev:
36.97/9.24
36.06/9.97
35.70/8.37
35.82/7.70
35.24/8.26


e10s disabled, accel enabled
============================
40.98/6.27
40.05/4.65
40.08/5.74
40.04/5.30
41.87/9.46

e10s disabled, accel disabled
=============================
49.16/9.6
47.03/6.99
46.59/7.89
47.03/8.55
46.02/8.74

No significant visual differences in the non-e10s case.
Should we just disable acceleration on e10s? Otherwise we should get an assignee
Flags: needinfo?(milan)
Mason, is vsync/silk relevant in this context?
Flags: needinfo?(mchang)
D3D9 vs. basic OMTC are fairly close together on lower resolutions; the higher the resolution, the worse D3D9 gets.
Silk might be. Can you test again on mozilla-beta or release and see if you produce the same results?

Then go to about:config and set the three preferences gfx.vsync.* to false and see what you get?

Also, a profile would help a lot here, then we could see how we're scheduling things. Thanks.
Flags: needinfo?(mchang) → needinfo?(vladan.bugzilla)
(Reporter)

Comment 11

2 years ago
The test resolution for the original finding was 1366x768 (on Win8.1 with D3D9 forced), Aaron reproduced the finding on a WinXP desktop with an external 1920x1080 monitor.

> Can you test again on mozilla-beta or release and see if you produce the same results?

The test hardware is in Toronto and it seems like we could cut out some indirection by having the Toronto gfx team test out some of these hypotheses.

> Should we just disable acceleration on e10s? Otherwise we should get an assignee

Jeff has some ideas on what could be causing this, so I think we should understand it
Flags: needinfo?(vladan.bugzilla)
(Reporter)

Updated

2 years ago
Blocks: 1172014
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #5)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #4)
> > Is this a regression on XP (which is the only platform where D3D9 is
> > supported)?
> 
> Correct. We don't want to put Windows XP on our reference hardware so it was
> tested on Windows 8 by forcing D3D9 via prefs.

The usual definition of "regression" is "used to work well in the past but now it doesn't work well".

By this definition, it's not a regression.

The data which this bug presents is that D3D9 performs worse than without acceleration on facebook. Whether or not it should actually be fixed is yet to be determined.
Note that Windows XP doesn't have a compositor, so Silk probably isn't going to look great there - as far as I know we fall back to a simple best effort 60Hz timer in that case. We use D3DPRESENT_INTERVAL_DEFAULT [1][2] for D3D9, which means Present will block and copy the back buffer to the screen during VSync, and that might interact badly with a timer that isn't synced up to it.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/DeviceManagerD3D9.cpp#264
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d9/DeviceManagerD3D9.cpp#680
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #13)
> during VSync

During VBlank, even.

Updated

2 years ago
Flags: needinfo?(milan)
Blocks: 1063169
tracking-e10s: ? → +
Priority: -- → P5
Benoit, can you profile this (on the actual XP machine, we have it), and see what's going on (e.g., comment 2.)
Flags: needinfo?(bgirard)
Assignee: nobody → bgirard
Actually, profile this on the non-XP machine, let's find out what the D3D9 performance problems are.

E10S and (real) XP with acceleration seems to be broken.  I will open a separate bug for that, but a summary - we inadvertently and unknowingly until now, turned off D3D9 for all of XP in 36.  We then, also inadvertently, and also unknowingly until now, fixed this in 41, enabling D3D9 with XP.  In the meantime, E10S got enabled, and broke XP+D3D9 combination.  Don't know if this is just one some hardware or not, will sort that out.
(Assignee)

Comment 17

2 years ago
I'm taking a look. We switched to 64-bit by default and the profiler doesn't support that. I'd like to fix that first.
(Assignee)

Comment 18

2 years ago
I can reproduce the problem trivial. 'Basic Content + D3D9 Layers' drops down to 30 FPS a lot while 'Basic Content + Basic Layers' stays near 60 FPS. Here's the profile during the 30 FPS dip:

https://cleopatra.io/#report=d36a4f0544e33ba128220d142c1865da04f0e0af&filter=[{%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A8464,%22end%22%3A9515}]

The compositor is taking too long to update the new texture data and blocks the rendering. Our texture upload are a lot slower than they should be even for a fullscreen update. Keeping ni? to investigate further.
(Assignee)

Updated

2 years ago
Depends on: 1240190
(Assignee)

Comment 19

2 years ago
Some progress today. We're not recycling anything. We're just allocating a full new texture (layer) and uploading everything every time sometime changes.

Working on a fix to recycle properly like D3D11.
(Assignee)

Comment 20

2 years ago
Created attachment 8711139 [details] [diff] [review]
Recycle texture and do partial uploads
Flags: needinfo?(bgirard)
Attachment #8711139 - Flags: review?(bas)
Comment on attachment 8711139 [details] [diff] [review]
Recycle texture and do partial uploads

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

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +320,5 @@
>                      : mTexture;
>  }
>  
> +static DXGI_FORMAT
> +D3D9SurfaceFormatToDXGIFormat(gfx::SurfaceFormat aFormat)

This doesn't take a D3D9 surface format as input but a Moz2D surface format.

@@ +356,5 @@
>      return false;
>    }
> +
> +  uint32_t bpp = BytesPerPixel(aSurface->GetFormat());
> +  DXGI_FORMAT dxgiFormat = D3D9SurfaceFormatToDXGIFormat(aSurface->GetFormat());

I'm guessing this is from dead testing code, you're not using this dxgiFormat variable, as a matter of fact DXGI_FORMAT isn't even a thing in D3D9.

@@ +402,2 @@
>      if (!mTexture) {
> +      // TODO Improve reallocating this texture is costly enough

nit: fix grammar

@@ +403,5 @@
> +      // TODO Improve reallocating this texture is costly enough
> +      //      that it causes us to skip frames on scrolling
> +      //      important pages like Facebook.
> +      mTexture = deviceManager->CreateTexture(mSize, format, D3DPOOL_DEFAULT, this);
> +      OutputDebugString(L"New Texture\n");

nit: leftover debug code. If you want to keep this in use gfxDebug()

@@ +424,5 @@
>      }
> +
> +    nsIntRegion regionToUpdate = aDestRegion ? *aDestRegion : nsIntRegion(nsIntRect(0, 0, mSize.width, mSize.height));
> +
> +    RefPtr<IDirect3DTexture9> srcTexture = deviceManager->CreateTexture(mSize, format, D3DPOOL_SYSTEMMEM, this);

Use CreateOffscreenPlainSurface, should be faster and saves some code. (https://msdn.microsoft.com/en-us/library/windows/desktop/bb174358%28v=vs.85%29.aspx)
Attachment #8711139 - Flags: review?(bas) → review-
(Assignee)

Comment 22

2 years ago
(In reply to Bas Schouten (:bas.schouten) from comment #21)
> @@ +424,5 @@
> >      }
> > +
> > +    nsIntRegion regionToUpdate = aDestRegion ? *aDestRegion : nsIntRegion(nsIntRect(0, 0, mSize.width, mSize.height));
> > +
> > +    RefPtr<IDirect3DTexture9> srcTexture = deviceManager->CreateTexture(mSize, format, D3DPOOL_SYSTEMMEM, this);
> 
> Use CreateOffscreenPlainSurface, should be faster and saves some code.
> (https://msdn.microsoft.com/en-us/library/windows/desktop/bb174358%28v=vs.
> 85%29.aspx)

Ahh neat. I was looking for something like that but missed it. Thanks!
(Assignee)

Comment 23

2 years ago
Created attachment 8711904 [details]
MozReview Request: Bug 1213429 - Recycle e10s D3D9 textures and add partial upload support. r=Bas

Review commit: https://reviewboard.mozilla.org/r/32369/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32369/
(Assignee)

Updated

2 years ago
Attachment #8711904 - Flags: review?(bas)
Comment on attachment 8711904 [details]
MozReview Request: Bug 1213429 - Recycle e10s D3D9 textures and add partial upload support. r=Bas

https://reviewboard.mozilla.org/r/32369/#review29115
Attachment #8711904 - Flags: review?(bas) → review+

Comment 25

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/066450137021

Comment 26

2 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efd7573d481b
(Assignee)

Comment 27

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09e538863879
(Assignee)

Comment 28

2 years ago
This patch got bitrotted by bug 1239864.
(Assignee)

Comment 29

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fbe8a191761
(Assignee)

Comment 30

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45edb7b76d49
(Assignee)

Comment 31

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=99d1bf5d3729
(Assignee)

Comment 32

2 years ago
Created attachment 8715493 [details] [diff] [review]
patch

This fixes XP reftest failures caused by A8 textures.
Attachment #8711139 - Attachment is obsolete: true
Attachment #8715493 - Flags: review+
(Assignee)

Comment 33

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=868e85e3c733
(Assignee)

Comment 34

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2003ba093ba23df0c7998dcbb3f71875537b9075
Bug 1213429 - Recycle e10s D3D9 textures and add partial upload support. r=Bas

Comment 35

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2003ba093ba2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(In reply to Benoit Girard (:BenWa) from comment #34)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 2003ba093ba23df0c7998dcbb3f71875537b9075
> Bug 1213429 - Recycle e10s D3D9 textures and add partial upload support.
> r=Bas

I don't know how I r+'ed this but it seems I wasn't paying attention on my second review, you're not checking the result of the LockRect call which could lead to a write to random memory, please fix!
Flags: needinfo?(bgirard)
(Assignee)

Comment 37

2 years ago
Alright, it was NULL when it failed but I'll fix it in a follow-up anyways.
(Assignee)

Comment 38

2 years ago
Created attachment 8716380 [details] [diff] [review]
patch
Attachment #8711904 - Attachment is obsolete: true
Attachment #8716380 - Flags: review?(bas)
Attachment #8716380 - Flags: review?(bas) → review+
(Assignee)

Comment 39

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c45f40620754ceb3ae0319f7b9b6255660146ac
Bug 1213429 - Check the result of LockRect. r=Bas
https://hg.mozilla.org/mozilla-central/rev/0c45f4062075

Comment 41

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ff849aa65c
https://hg.mozilla.org/mozilla-central/rev/d6ff849aa65c
(Assignee)

Updated

a year ago
Flags: needinfo?(bgirard)
You need to log in before you can comment on or make changes to this bug.