1393031 - Batch resource updates in transactions.

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Nicolas Silva [:nical]]

WebRender now exposes resource updates in a way that lets us batch them. This is important to respect transactions, otherwise two pipelines racing each other can cause a frame to be generated with a subset of the updates of a transaction.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Expose webrender's ResourceUpdates in the C++ API

This is a preliminary patch that starts separating WebRenderAPI and the resource updates.

The idea is that ResourceUpdateQueue accumulates the resource updates that have to be applied within a given transaction and webrender ensures they are applied atomically.

For now, in order to avoid churning too much code and colliding with Jerry's work, the resource updates are exposed as a member of the WebRenderAPI, but it's not entirely clear whether it should stay that way.
because the bindings generator already produces a placeholder struct named ResourceUpdates that corresponds to the rust type, I named the C++ class that wraps it ResourceUpdateQueue (i'd feel better if we had maybe an ffi namespace for the rust ffi types so that we don't have to bother with finding a different name for the C++ wrapper, but we can worry deal with that later).

The next step is to extend this to the child process and have it batch up the resource updates and send them as part of the transaction as well.

Comment 2

[Nicolas Silva [:nical]]

Comment on attachment 8900266 [details] [diff] [review]
Expose webrender's ResourceUpdates in the C++ API

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

::: gfx/layers/wr/WebRenderBridgeParent.cpp
@@ +1225,5 @@
>  
>  void
>  WebRenderBridgeParent::DeleteOldImages()
>  {
>    for (wr::ImageKey key : mKeysToDelete) {

Since the ResourceUpdates struct already buffers key deletion until the transaction we may not need to use mKeysToDelete anymore, but I want to experiment with that separately.

Comment 3

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8900266 [details] [diff] [review]
Expose webrender's ResourceUpdates in the C++ API

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

::: gfx/webrender_bindings/WebRenderAPI.cpp
@@ +252,5 @@
>  void
>  WebRenderAPI::ClearRootDisplayList(Epoch aEpoch,
>                                     wr::WrPipelineId pipeline_id)
>  {
>    wr_api_clear_root_display_list(mDocHandle, aEpoch, pipeline_id);

Is there a reason why it does not pass mResources.Raw() like wr_api_set_display_list()?

::: gfx/webrender_bindings/src/bindings.rs
@@ +826,4 @@
>  }
>  
>  #[no_mangle]
>  pub unsafe extern "C" fn wr_api_clear_root_display_list(dh: &mut DocumentHandle,

You might want to remove "root" from the function name like the above wr_api_set_display_list().

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> Comment on attachment 8900266 [details] [diff] [review]
> Expose webrender's ResourceUpdates in the C++ API
> 
> Review of attachment 8900266 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/webrender_bindings/WebRenderAPI.cpp
> @@ +252,5 @@
> >  void
> >  WebRenderAPI::ClearRootDisplayList(Epoch aEpoch,
> >                                     wr::WrPipelineId pipeline_id)
> >  {
> >    wr_api_clear_root_display_list(mDocHandle, aEpoch, pipeline_id);
> 
> Is there a reason why it does not pass mResources.Raw() like
> wr_api_set_display_list()?

nical: can you answer the above? The patch look OK for me except it.

Comment 5

[Nicolas Silva [:nical]]

> Is there a reason why it does not pass mResources.Raw() like
> wr_api_set_display_list()?

Good point, I'll update the patch with this and the other comment.

Comment 6

[Nicolas Silva [:nical]]

Comment on attachment 8900266 [details] [diff] [review]
Expose webrender's ResourceUpdates in the C++ API

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

::: gfx/webrender_bindings/src/bindings.rs
@@ +837,5 @@
>                              None,
>                              LayoutSize::new(0.0, 0.0),
>                              frame_builder.dl_builder.finalize(),
>                              preserve_frame_state,
> +                            ResourceUpdates::new()); // TODO

Huh, I even had a TODO about it and forgot.

Comment 7

[Nicolas Silva [:nical]]

Attachment: Expose webrender's ResourceUpdates in the C++ API (v2)

There we go.

Comment 8

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8900624 [details] [diff] [review]
Expose webrender's ResourceUpdates in the C++ API (v2)

Looks good!

Comment 9

[Nicolas Silva [:nical]]

Attachment: Make ResourceUpdateQueue move-only

This makes sure we don't accidentally cause havoc by trying to assign things around. I realize that I am specifying more than I need to (some ctors and operators would be already disabled by the presence of others), but since I had to look it up to be sure I felt it would be easier to everyone if I erase any possible doubt using extra explicitness.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/326d1e6cf7b1
Expose WebRender's ResourceUpdates to C++. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/663105088e1e
Make ResourceUpdateQueue move-only. r=sotaro

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for bustage at gfx/layers/opengl/MacIOSurfaceTextureHostOGL.cpp:178: no member named 'AddExternalImage' in 'mozilla::wr::WebRenderAPI' on OS X and also bustage on Windows:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b82b1c3c9f95bf94817d20cb763d90a58771b65
https://hg.mozilla.org/integration/mozilla-inbound/rev/f833f336b2c9e547dd71200685bda4aa078bbcd6

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=663105088e1eec5048b04d5a03faffb9f8b13c55&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log OS X: https://treeherder.mozilla.org/logviewer.html#?job_id=125849012&repo=mozilla-inbound
> /home/worker/workspace/build/src/gfx/layers/opengl/MacIOSurfaceTextureHostOGL.cpp:178:13: error: no member named 'AddExternalImage' in 'mozilla::wr::WebRenderAPI'
etc.

Failure log Windows: https://treeherder.mozilla.org/logviewer.html#?job_id=125849014&repo=mozilla-inbound
10:11:41     INFO -  Unified_cpp_netwerk_base0.cpp
10:11:41     INFO -  z:\build\build\src\obj-firefox\dist\include\mozilla/webrender/WebRenderAPI.h(107): error C2220: warning treated as error - no 'object' file generated
10:11:41     INFO -  z:\build\build\src\obj-firefox\dist\include\mozilla/webrender/WebRenderAPI.h(107): warning C4521: 'mozilla::wr::ResourceUpdateQueue': multiple copy constructors specified
10:11:41     INFO -  z:\build\build\src\obj-firefox\dist\include\mozilla/webrender/WebRenderAPI.h(107): warning C4522: 'mozilla::wr::ResourceUpdateQueue': multiple assignment operators specified
10:11:41     INFO -  z:/build/build/src/config/rules.mk:1064: recipe for target 'Unified_cpp_netwerk_base0.obj' failed
10:11:41     INFO -  mozmake.EXE[5]: *** [Unified_cpp_netwerk_base0.obj] Error 2

Comment 12

[Nicolas Silva [:nical]]

Attachment: Stop tracking active resource keys on the compositor side.

By the time I am done with this bug, we'll pass the complete serialized ResourceUpdateQueue directly from the content and we won't have individual AddImage/UpdateImage/etc messages. In order to avoid leaks when a process crashes we should use webrender's clear_namespace method instead of tracking live resources manually.

Comment 13

[Nicolas Silva [:nical]]

Attachment: [WIP] Expose webrender transactions in the IPC layer.

This does not yet remove the ipc messages to add/update/remove images and fonts, but adds the plumbing to serialize resource updates using bincode to build it on the content side thoruh IPDL.
It also removes the totally useless DPBegin message, and renames DPEnd into SetDisplayList in the ipc protocol.

Comment 14

[Nicolas Silva [:nical]]

Attachment: [WIP] Expose webrender transactions in the IPC layer.

Comment 15

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8901187 [details] [diff] [review]
Stop tracking active resource keys on the compositor side.

Yes, we could remove them now :)

Comment 16

[Nicolas Silva [:nical]]

Attachment: Expose webrender transactions in the IPC layer.

Comment 17

[Nicolas Silva [:nical]]

Attachment: Use the ResourceUpdateQueue API in the content process

Comment 18

[Nicolas Silva [:nical]]

Next up is external images.

It's not as straightforward as regular images and fonts because some code has to happen on the compositor so we can't just record the AddExternalImage calls on the client side and pass them directly to webrender like we do for regular images. We still do need to have them in the transaction so I am thinking of not using the ResourceUpdateQueue for that on the content side and build a list of the external image commands by talking to either WebRenderBridge child or something stored in the displaylist builder. Then that list has to be sent in he same ipc message (SetDisplayList, UpdateResource etc.) to make sure it is properly applied in the transaction (the point of all of this) and on the compositor side, do the glue that needs to be done and add merge the external image related updates with other updates in the ResourceUpdateQueue on that side before sending the transaction to WebRender.
Alternatively (small variation), add the updates in the ResourceUpdate Queue on the content side and maintain a separate list of whatever information the compositor will need to do its glue (basically same thing but merge the resource updates on the child side instead of the compositor).

Comment 19

[Jeff Muizelaar [:jrmuizel]]

It seems like this introduces an additional copy of the image/font data. Am I correct in this assumption?

Comment 20

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #19)
> It seems like this introduces an additional copy of the image/font data. Am
> I correct in this assumption?

Actually, two new copies. One on the way in and one on the way out.

Comment 21

[Jeff Muizelaar [:jrmuizel]]

It might be possible to solve the extra copy problem by using ParamTraits for ResourceUpdates that call into bincode of bincoding into a ByteBuf.

Comment 22

[Nicolas Silva [:nical]]

> It seems like this introduces an additional copy of the image/font data. Am I correct in this assumption?

Correct. It's pretty much the same thing as with regular display lists, so I'd like that we handle both the same way (for example encode the resource updates as we build them as we do for display items, or do the bincode in ParamTraits for both if that's better).

Anyway, one step at a time. There is already a lot to chew on this bug.

Comment 23

[Nicolas Silva [:nical]]

Attachment: Separate WebRenderAPI and ResourceUpdateQueue.

For convenience, the first patch was storing a ResourceUpdateQueue in the WebRenderAPI object. I wasn't very happy about it and it led to some confusing code and a bug where I was passing the wrong queue to WebRender.
This patch removes the ResourceUpdateQueue member and passes the UpdateQueue directly to the various texture host methods which don't need full access to the wr API anyway.

This will be folded into the first patch but I kept it separate to make reviewing a bit easier.

Comment 24

[Nicolas Silva [:nical]]

Attachment: Apply external image commands in the webrender transaction

This patch makes it so the webrender parent commands which contain external image logic apply in the same transaction as the other commands when calling SetDisplayList.

Comment 25

[Nicolas Silva [:nical]]

Attachment: Remove DeleteOldImages

We don't use mKeysToDelete anymore (with the the other patches applied).

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73cbc76a296b
Expose webrender resource updates in the C++ wrapper. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/684dd3236401
Make ResourceUpdateQueue move-only. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf18746c20ea
Stop tracking active resource keys on the parent side. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/44463d723486
Expose webrender transactions at the ipc boundary. r=jrmuizel, r=kanru
https://hg.mozilla.org/integration/mozilla-inbound/rev/36260b6ee764
Use the ResourceUpdateQueue API on the content side. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/959919f16fe2
Separate WebRenderAPI and ResourceUpdateQueue. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb5cd2dd689
Apply external image commands in webrender transactions. r=sotaro

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/73cbc76a296b
https://hg.mozilla.org/mozilla-central/rev/684dd3236401
https://hg.mozilla.org/mozilla-central/rev/cf18746c20ea
https://hg.mozilla.org/mozilla-central/rev/44463d723486
https://hg.mozilla.org/mozilla-central/rev/36260b6ee764
https://hg.mozilla.org/mozilla-central/rev/959919f16fe2
https://hg.mozilla.org/mozilla-central/rev/7cb5cd2dd689

Comment 28

[Nicolas Silva [:nical]]

Attachment: Add IpcResourceUpdates and use Shmem to pass resources

This patch is awfully big I am sorry. Thankfully, it's largely bloated by a tedious addition of a parameter in a gazillion of overloads of the same methods so the overall patch isn't very complicated. I'll highlight the important parts inline.

in a nutshell, this patch:
 - Adds IpcResourceUpdateQueue which is like ResourceUpdateQueue but that is optimized to be passed to ipdl. the later writes blobs of bytes in shared memory to avoid a handful of copies happening all over ipdl.
 - separates DisplaylistBuilder from (Ipc)ResourceUpdateQueue: I initially glued them together out of laziness, but I've never felt really good about it. now that the update queue we use is the ipdl one, I prefer to keep them separate like they are in webrender's API, and avoid having the c++ wrappers around webrender types depending on ipdl stuff. This is what caused the addition of the paramter that constitute half of the patch.

As a followup I want to merge the infamous nsTArray<WebRenderParentMessage> which is used to send external image stuff with IpcResourceUpdateQueue (but that's the next patch because this one is too big already).

Comment 29

[Nicolas Silva [:nical]]

Attachment: Add IpcResourceUpdates and use Shmem to pass resources

Comment 30

[Nicolas Silva [:nical]]

Comment on attachment 8906001 [details] [diff] [review]
Add IpcResourceUpdates and use Shmem to pass resources

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

::: gfx/layers/wr/IpcResourceUpdateQueue.cpp
@@ +23,5 @@
> +  Clear();
> +}
> +
> +layers::OffsetRange
> +ShmSegmentsWriter::Write(Range<uint8_t> aBytes)

This deserves a careful review.

@@ +120,5 @@
> +  }
> +}
> +
> +bool
> +ShmSegmentsReader::Read(layers::OffsetRange aRange, wr::Vec_u8& aInto)

This deserves a careful review.

::: gfx/webrender_bindings/src/bindings.rs
@@ +87,5 @@
>          unsafe { Vec::from_raw_parts(self.data, self.length, self.capacity) }
>      }
> +
> +    // Equivalent to `to_vec` but clears self instead of consuming the value.
> +    fn flush_into_vec(&mut self) -> Vec<u8> {

This deserves a careful review, and the stuff below as well.

@@ +676,5 @@
>  pub extern "C" fn wr_resource_updates_add_image(
>      resources: &mut ResourceUpdates,
>      image_key: WrImageKey,
>      descriptor: &WrImageDescriptor,
> +    bytes: &mut WrVecU8,

Note: a bunch of these functions have been changed from taking a slice to taking a Vec<u8>. this saves a copy \o/

Comment 31

[Jeff Muizelaar [:jrmuizel]]

(In reply to Nicolas Silva [:nical] from comment #28)
> in a nutshell, this patch:
>  - Adds IpcResourceUpdateQueue which is like ResourceUpdateQueue but that is
> optimized to be passed to ipdl. the later writes blobs of bytes in shared
> memory to avoid a handful of copies happening all over ipdl.
>  - separates DisplaylistBuilder from (Ipc)ResourceUpdateQueue: I initially
> glued them together out of laziness, but I've never felt really good about
> it. now that the update queue we use is the ipdl one, I prefer to keep them
> separate like they are in webrender's API, and avoid having the c++ wrappers
> around webrender types depending on ipdl stuff. This is what caused the
> addition of the paramter that constitute half of the patch.


This sounds reasonable in principle. I'll try to take a closer look soon.

Comment 32

[Nicolas Silva [:nical]]

Attachment: Add IpcResourceUpdates and use Shmem to pass resources

Rebased plus a few fixes.

Comment 33

[Nicolas Silva [:nical]]

Attachment: Begin moving OpAddExternalImage to IpcResourceUpdateQueue

This patch begins moving stuff from the WebRenderParentMessage to IpcResourceUpdateQueue.

In the process I moved WebRenderLayer::UpdateImageKey to WebRenderImageLayer since it is the only user, and made it recycle the key instead of throwing it away each time something changes.

Again, a big part of the patch is tedious but trivial plumbing to pass the update queue as parameter in various places.

Comment 34

[Nicolas Silva [:nical]]

Attachment: Begin moving OpAddExternalImage to IpcResourceUpdateQueue

I had to fiddle a bit with the ordering of things because some messages are still in the WebRenderParentMessage thing and must be processed before the OpAddExternalImage in the resource update queue. That's why WebRenderBridgeParent::ProcessWebRenderCommands got inlined into its only caller (RecvSetDisplayList).

Comment 35

[Nicolas Silva [:nical]]

Attachment: Use IpcResourceUpdateQueue in more places

By now I can type "wr::IpcResourceUpdateQueue& aResources," faster than my own name.

Comment 36

[Nicolas Silva [:nical]]

Attachment: Remove OpAddExternalImage from WebRenderParentCommands.

It's now all going through IpcResourceUpdateQueue.

Comment 37

[Nicolas Silva [:nical]]

Attachment: Remove the unused aPrentCommands parameter all over the code.

This just removes the unused parameter spread all over the display list building code.

This is the last patch for this bug \o/. There's still some followup cleanups that should be done but it is less pressing and can be dealt with in separate bugs.

Comment 38

[Nicolas Silva [:nical]]

With this patch queue we have most of the work in place to fix the frame consistency issues (and already have fixed a lot of them), but their are still the places where we use PImageBridge to pass resources that should be part of the transaction to fix before we have correct frame consistency, like Bug 1396588.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/131084e007d6
Remove WebRenderBridgeParent::DeleteOldImages. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b78bddcd706
Use shared memory to pass resource update data. r=jrmuizel

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67ca40453027
Begin moving OpAddExternalImage to IpcResourceUpdateQueue and recycle image keys in WebRenderImageLayer. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/8291145f0920
Use IpcResourceUpdateQueue in more places. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/980729ab6e94
Remove OpAddExternalImage from WebRenderParentCommands. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/f34a52244cf9
Remove unused nsTArray<WebRenderParentCommand>& parameter all over the place. r=jrmuizel

Comment 41

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing e.g. reftest layout/reftests/image/image-srcset-basic-selection-2x-ref.html on QuantumRender_

https://hg.mozilla.org/integration/mozilla-inbound/rev/edddc72cb9a8b62fa6e921332c32128594fede6c
https://hg.mozilla.org/integration/mozilla-inbound/rev/3428a9f92bf4bd18ee78946834a201a670a54752
https://hg.mozilla.org/integration/mozilla-inbound/rev/80142d2029b6d66a281dfd92d44bb08d28b86737
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1d623b0acedbc5ebc19a7a43a09a30d8e2af702

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f34a52244cf9f7d75b55edb2ca4bbe28ce5717ee&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=131118468&repo=mozilla-inbound
> REFTEST TEST-UNEXPECTED-FAIL | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/image/image-srcset-basic-selection-2x.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/image/image-srcset-basic-selection-2x-ref.html | image comparison, max difference: 50, number of differing pixels: 40000

Comment 42

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/131084e007d6
https://hg.mozilla.org/mozilla-central/rev/0b78bddcd706

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a937c7957c5f
Begin moving OpAddExternalImage to IpcResourceUpdateQueue and recycle image keys in WebRenderImageLayer. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/65f9f54bd7bf
Use IpcResourceUpdateQueue in more places. r=jrmuizel

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ecd18f10203
Remove OpAddExternalImage from WebRenderParentCommands. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c407eb7c7ef
Remove unused nsTArray<WebRenderParentCommand>& parameter all over the place. r=jrmuizel

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a937c7957c5f
https://hg.mozilla.org/mozilla-central/rev/65f9f54bd7bf
https://hg.mozilla.org/mozilla-central/rev/0ecd18f10203
https://hg.mozilla.org/mozilla-central/rev/5c407eb7c7ef