Closed Bug 1393031 Opened 7 years ago Closed 7 years ago

Batch resource updates in transactions.

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- unaffected
firefox57 --- unaffected

People

(Reporter: nical, Assigned: nical)

References

Details

(Whiteboard: [wr-mvp])

Attachments

(13 files, 6 obsolete files)

57.97 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
2.29 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
6.58 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
42.49 KB, patch
jrmuizel
: review+
kanru
: review+
Details | Diff | Splinter Review
26.28 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
40.59 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
5.67 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
4.92 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
192.89 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
44.53 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
36.84 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.25 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
103.50 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #8900266 - Flags: review?(sotaro.ikeda.g)
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 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().
Attachment #8900266 - Flags: review?(sotaro.ikeda.g)
(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.
Flags: needinfo?(nical.bugzilla)
> 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.
Flags: needinfo?(nical.bugzilla)
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.
There we go.
Attachment #8900266 - Attachment is obsolete: true
Attachment #8900624 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8900624 [details] [diff] [review]
Expose webrender's ResourceUpdates in the C++ API (v2)

Looks good!
Attachment #8900624 - Flags: review?(sotaro.ikeda.g) → review+
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.
Attachment #8900640 - Flags: review?(sotaro.ikeda.g)
Attachment #8900640 - Flags: review?(sotaro.ikeda.g) → review+
Pushed by nsilva@mozilla.com:
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
Keywords: leave-open
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
Flags: needinfo?(nical.bugzilla)
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.
Flags: needinfo?(nical.bugzilla)
Attachment #8901187 - Flags: review?(sotaro.ikeda.g)
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.
Attachment #8901189 - Attachment is obsolete: true
Comment on attachment 8901187 [details] [diff] [review]
Stop tracking active resource keys on the compositor side.

Yes, we could remove them now :)
Attachment #8901187 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8901264 - Attachment is obsolete: true
Attachment #8902247 - Flags: review?(jmuizelaar)
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).
It seems like this introduces an additional copy of the image/font data. Am I correct in this assumption?
Flags: needinfo?(nical.bugzilla)
(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.
It might be possible to solve the extra copy problem by using ParamTraits for ResourceUpdates that call into bincode of bincoding into a ByteBuf.
> 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.
Flags: needinfo?(nical.bugzilla)
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.
Attachment #8902744 - Flags: review?(sotaro.ikeda.g)
Attachment #8902744 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8902247 - Flags: review?(jmuizelaar) → review+
Attachment #8902248 - Flags: review?(jmuizelaar) → review+
Attachment #8902247 - Flags: review?(kchen)
Attachment #8902247 - Flags: review?(kchen) → review+
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.
Attachment #8904204 - Flags: review?(sotaro.ikeda.g)
We don't use mKeysToDelete anymore (with the the other patches applied).
Attachment #8904219 - Flags: review?(sotaro.ikeda.g)
Pushed by nsilva@mozilla.com:
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
Depends on: 1397220
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [wr-mvp]
Target Milestone: --- → mozilla57
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).
Attachment #8906000 - Flags: review?(jmuizelaar)
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/
Attachment #8906001 - Flags: review?(jmuizelaar)
Attachment #8906000 - Attachment is obsolete: true
Attachment #8906000 - Flags: review?(jmuizelaar)
(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.
Rebased plus a few fixes.
Attachment #8906001 - Attachment is obsolete: true
Attachment #8906001 - Flags: review?(jmuizelaar)
Attachment #8907023 - Flags: review?(jmuizelaar)
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.
Attachment #8907053 - Flags: review?(jmuizelaar)
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).
Attachment #8907053 - Attachment is obsolete: true
Attachment #8907053 - Flags: review?(jmuizelaar)
Attachment #8907085 - Flags: review?(jmuizelaar)
By now I can type "wr::IpcResourceUpdateQueue& aResources," faster than my own name.
Attachment #8907169 - Flags: review?(jmuizelaar)
Attachment #8907023 - Flags: review?(jmuizelaar) → review+
Attachment #8907085 - Flags: review?(jmuizelaar) → review+
It's now all going through IpcResourceUpdateQueue.
Attachment #8907591 - Flags: review?(jmuizelaar)
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.
Attachment #8907596 - Flags: review?(jmuizelaar)
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.
Attachment #8907596 - Flags: review?(jmuizelaar) → review+
Attachment #8907591 - Flags: review?(jmuizelaar) → review+
Attachment #8907169 - Flags: review?(jmuizelaar) → review+
Attachment #8904219 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8904204 - Flags: review?(sotaro.ikeda.g) → review+
Pushed by nsilva@mozilla.com:
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
Pushed by nsilva@mozilla.com:
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
Depends on: 1400532
Pushed by nsilva@mozilla.com:
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
Pushed by nsilva@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(nical.bugzilla)
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.