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)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla47
People
(Reporter: vladan, Assigned: BenWa)
References
(Blocks 1 open bug)
Details
(Whiteboard: [content perf][gfx-noted])
Attachments
(2 files, 2 obsolete files)
5.50 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
1.38 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
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
Comment 2•9 years ago
|
||
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•9 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]
Comment 4•9 years ago
|
||
Is this a regression on XP (which is the only platform where D3D9 is supported)?
Flags: needinfo?(vladan.bugzilla)
Reporter | ||
Comment 5•9 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)
Comment 6•9 years ago
|
||
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.
Comment 7•9 years ago
|
||
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.
Comment 10•9 years ago
|
||
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•9 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)
Comment 12•9 years ago
|
||
(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.
Comment 13•9 years ago
|
||
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
Comment 14•9 years ago
|
||
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #13) > during VSync During VBlank, even.
Updated•8 years ago
|
Flags: needinfo?(milan)
Updated•8 years ago
|
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)
Updated•8 years ago
|
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•8 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•8 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 | ||
Comment 19•8 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•8 years ago
|
||
Flags: needinfo?(bgirard)
Attachment #8711139 -
Flags: review?(bas)
Comment 21•8 years ago
|
||
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•8 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•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/32369/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/32369/
Assignee | ||
Updated•8 years ago
|
Attachment #8711904 -
Flags: review?(bas)
Comment 24•8 years ago
|
||
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 26•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/efd7573d481b
Assignee | ||
Comment 27•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09e538863879
Assignee | ||
Comment 28•8 years ago
|
||
This patch got bitrotted by bug 1239864.
Assignee | ||
Comment 29•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fbe8a191761
Assignee | ||
Comment 30•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45edb7b76d49
Assignee | ||
Comment 31•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=99d1bf5d3729
Assignee | ||
Comment 32•8 years ago
|
||
This fixes XP reftest failures caused by A8 textures.
Attachment #8711139 -
Attachment is obsolete: true
Attachment #8715493 -
Flags: review+
Assignee | ||
Comment 33•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=868e85e3c733
Assignee | ||
Comment 34•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2003ba093ba2
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment 36•8 years ago
|
||
(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•8 years ago
|
||
Alright, it was NULL when it failed but I'll fix it in a follow-up anyways.
Assignee | ||
Comment 38•8 years ago
|
||
Attachment #8711904 -
Attachment is obsolete: true
Attachment #8716380 -
Flags: review?(bas)
Updated•8 years ago
|
Attachment #8716380 -
Flags: review?(bas) → review+
Assignee | ||
Comment 39•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c45f40620754ceb3ae0319f7b9b6255660146ac Bug 1213429 - Check the result of LockRect. r=Bas
Comment 40•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0c45f4062075
Comment 42•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d6ff849aa65c
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(bgirard)
You need to log in
before you can comment on or make changes to this bug.
Description
•