Closed Bug 1360766 Opened 3 years ago Closed 3 years ago

Pre-initialize DeviceAttachmentsD3D11

Categories

(Core :: Graphics: Layers, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(3 files)

Currently we create DeviceAttachmentsD3D11 on demand when we create the first compositor, and destroy it if there are no compositors left. This has the disadvantage that the compositor thread sits idle until a PLayerTransaction request comes in, then it has to load a bunch of shaders which is very expensive.

Instead, we should create the attachments when the GPU process initializes, so they're ready by the time the first compositor request comes in.

I have before [1] and after [2] profile comparisons. In the first profile, the GetTextureFactoryIdentifier request takes 450ms because we're waiting for the PLayerTransaction constructor to finish. Meanwhile the GPU process has been idle for 3 seconds.

In the second profile, the GPU process initializes the D3D11 attachments almost immediately after startup, and is done long before the PLayerTransaction message comes in. 

[1] https://perf-html.io/public/00234e7596acca431abb7ee0e336d7f763ec0869/calltree/?thread=3
[2] https://perf-html.io/public/dd30a3f53ed5d2f5bae932db0bc01040de626adc/calltree/?callTreeFilters=prefix-01234567&thread=4
Note, those profiles were taken on the Acer reference machine.
This moves DeviceAttachmentsD3D11 into its own header and source file, like how MLGDevice is set up in advanced-layers.
Attachment #8863079 - Flags: review?(bas)
Also similar to MLGDevice, this moves the attachments to DeviceManagerDx. As a result they won't go away until shutdown or a device reset.
Attachment #8863080 - Flags: review?(bas)
This posts a task to the compositor thread, asking to initialize DeviceAttachmentsD3D11 before any compositors have been requested. This is done both for in-process and out-of-process compositing.
Attachment #8863081 - Flags: review?(bas)
Comment on attachment 8863079 [details] [diff] [review]
part 1, separate source/h file

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

::: gfx/layers/d3d11/DeviceAttachmentsD3D11.cpp
@@ +310,5 @@
> +
> +bool
> +DeviceAttachmentsD3D11::Failed(HRESULT hr, const char* aContext)
> +{
> +  if (SUCCEEDED(hr))

nit: While we're in here add {}

::: gfx/layers/d3d11/DeviceAttachmentsD3D11.h
@@ +20,5 @@
> +struct DeviceAttachmentsD3D11
> +{
> +  explicit DeviceAttachmentsD3D11(ID3D11Device* device);
> +
> +  bool Initialize(nsCString* const out_failureReason);

nit: Let's not perpetuate the silly, inconsistent underscore based variable naming and use aFailureReason or something like that.
Attachment #8863079 - Flags: review?(bas) → review+
Comment on attachment 8863080 [details] [diff] [review]
part 2, store attachments on DeviceManagerDx

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

::: gfx/thebes/DeviceManagerDx.cpp
@@ +925,5 @@
> +  // We save the attachments object even if it fails to initialize, so the
> +  // compositor can grab the failure ID.
> +  RefPtr<layers::DeviceAttachmentsD3D11> attachments =
> +    new layers::DeviceAttachmentsD3D11(device);
> +  attachments->Initialize();

I'd prefer making this DeviceAttachmentsD3D11::Create(device)
Attachment #8863080 - Flags: review?(bas) → review+
Attachment #8863081 - Flags: review?(bas) → review+
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01c184bb2a27
Separate DeviceAttachmentsD3D11 into its own header and source file. (bug 1360766 part 1, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa75fd2b360
Store DeviceAttachmentsD3D11 on DeviceManagerDx instead of ID3D11Device. (bug 1360766 part 2, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/154b84a2fd16
Pre-initialize DeviceAttachmentsD3D11 on the compositor thread, immediately after the GPU process initializes. (bug 1360766 part 3, r=bas)
Pushed w/ nits fixed. I had to move the in-process case later in gfxPlatform since it was being called before the compositor thread was started.
this has a win for startup time on windows 8:
== Change summary for alert #6296 (as of April 30 2017 18:41 UTC) ==

Improvements:

  2%  sessionrestore windows8-64 opt      817.54 -> 799.92
  2%  sessionrestore_no_auto_restore windows8-64 opt 835.42 -> 818.5

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6296
(In reply to Joel Maher ( :jmaher) from comment #10)
> this has a win for startup time on windows 8:
> == Change summary for alert #6296 (as of April 30 2017 18:41 UTC) ==
> 
> Improvements:
> 
>   2%  sessionrestore windows8-64 opt      817.54 -> 799.92
>   2%  sessionrestore_no_auto_restore windows8-64 opt 835.42 -> 818.5
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=6296

Great, I was hoping this would make a difference on Talos too. Unfortunately doesn't look like enough to recover from bug 1356138, yet.
You need to log in before you can comment on or make changes to this bug.