Closed Bug 1213429 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: Graphics, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
e10s + ---
firefox47 --- fixed

People

(Reporter: vladan, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files, 2 obsolete files)

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
See Also: → 1213432
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)
> 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)
(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)
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)
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.
Flags: needinfo?(milan)
Blocks: e10s-perf
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.
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.
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.
Depends on: 1240190
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.
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-
(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!
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+
This patch got bitrotted by bug 1239864.
Attached patch patchSplinter Review
This fixes XP reftest failures caused by A8 textures.
Attachment #8711139 - Attachment is obsolete: true
Attachment #8715493 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2003ba093ba23df0c7998dcbb3f71875537b9075
Bug 1213429 - Recycle e10s D3D9 textures and add partial upload support. r=Bas
https://hg.mozilla.org/mozilla-central/rev/2003ba093ba2
Status: NEW → RESOLVED
Closed: 8 years ago
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)
Alright, it was NULL when it failed but I'll fix it in a follow-up anyways.
Attached patch patchSplinter Review
Attachment #8711904 - Attachment is obsolete: true
Attachment #8716380 - Flags: review?(bas)
Attachment #8716380 - Flags: review?(bas) → review+
Flags: needinfo?(bgirard)
You need to log in before you can comment on or make changes to this bug.