1612440 - Send transactions directly to the scene builder thread when possible

Status: RESOLVED FIXED

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

Description

[Nicolas Silva [:nical]]

Currently transactions that trigger scene building first go through the render backend, then some resources are collected (font, blobs) and sent to the scene builder. If the render backend is busy this delays the scene being processed, causing extra latency for layout transactions.

With ipc_channel removed, we can move the few hash maps on that store data we send to the scene builder, keep them at the API level instead and send messages directly to the scene builder. It should make things simpler (no need to remember which data is pre/post scene building on the render backend, everything is post-scene building) and reduce latency in some cases.

Comment 1

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Wrap the shared font instance map to a type exposed in the api crate. r=gw

This is a small preliminary refactoring that will be needed to let the api talk to the scene builder thread.

Comment 2

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Don't implement Serialize/Deserialize in some parts of th API. r=gw

Now that the capture code is gone, we don't use these serde implementations. They get in the way of changing the way transactions are represented so I'd like to remove a bunch of them for now.

Comment 3

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Move some scene-related transaction handling code to the scene builder thread. r=gw

The goal is to move this code out of the render backend to avoid requiring the messages to transit through this thread. Other similar changes are in progress.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a307cb281ea1
Don't implement Serialize/Deserialize in some parts of th API. r=gw
https://hg.mozilla.org/integration/autoland/rev/d3387bfa2b02
Wrap the shared font instance map to a type exposed in the api crate. r=gw

Comment 5

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Separate scene and frame related data in DocumentView. r=gw

DocumentView has a mix of members that affect scene building and memebers that change frame to frame. These need to be updated at different rates and more importantly follow the respective flow of scene and frame transactions, which wasn't done properly before this change.

Depends on D71781

Comment 6

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Discard the SpatialTree's frame state after scene swap. r=kats

The information comes with SetDisplayList messages but was applied before scene building instead of during scene swap, which breaks the transaction model and looks like a bug.

Comment 7

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Move pre-scene-building resources to the RenderApi.

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/a307cb281ea1
https://hg.mozilla.org/mozilla-central/rev/d3387bfa2b02

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7675477d0507
Move some scene-related transaction handling code to the scene builder thread. r=gw
https://hg.mozilla.org/integration/autoland/rev/4933e4b66edb
Separate scene and frame related data in DocumentView. r=gw

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/7675477d0507
https://hg.mozilla.org/mozilla-central/rev/4933e4b66edb

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f1096e3894c
Discard the SpatialTree's frame state after scene swap. r=gw

Comment 12

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Remove ApiMsg::UpdateResources. r=gw

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2f1096e3894c

Comment 14

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Avoid re-building the scene in some cases. r=gw

Turns out we sometimes send transactions that don't change anything (for example in reftests we continuously set the quality setting to the same value) which can trigger unnecessary scene building. In reftests this causes us to build a scene after removing font instances that it refers to.

Comment 15

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Move SceneBuilderRequest to webrender_api. r=gw

This requires pulling FrameBuilderConfig and a few other types along which isn't very satisfying but oh well.

Comment 16

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Separate SceneBuilderRequest in two. r=gw,aosmond

This patch separates SceneBuilderRequest into two enums: One we'll be able to send from the api, and containing types that shouldn't move to the webrender_api crate. For example LoadScene depends on the Scene struct and at this point a quarter of webrender's types would need to move to webrender_api.
This solution is a bit iffy but I'm not sure how to better do this. If we don't need strict ordering, moving to crossbeam's queue which support select would let us at least avoid sending dummy BackendMessage to instruct the scene builder thread to look into the other queue.
This patch should not change the current behavior except for one (important) detail: all messages that use BackendSceneBuilderMessage have to go through the low priority scene queue instead of the high priority one. This affects the capture stuff, I think that it's still correct but I'm not familiar enough with it to be sure.

Comment 17

[Josh Matthews [:jdm]]

https://github.com/servo/webrender/pull/3936 adds the Serialize/Deserialize that allow Servo to build with the master revision of the webrender git repository. This will shortly be broken again by https://phabricator.services.mozilla.com/D72397 if that revision lands and syncs upstream.

Comment 18

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Forward glyph dimension and index requests through the scene builder thread. r=gw

This prevents the request from racing ahead of the AddFontInstance message which follows the transaction through the scene builder thread and silently returning zero-sized glyphs.

Comment 19

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Merge the webrender::Transaction and webrender_api::TransactionMsg structs. r=gw

One of the last steps towards letting the api send scene building requests directly. I'll clean up the names after everything is in place.

Depends on D72614

Comment 20

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Send some messages directly to the scene builder thread. r=gw

This change allows the api to talk directly to the scene builder thread. This avoids unpredictable and sometimes large latency caused by scene builder requests sitting in a busy render backend's event loop.

Depends on D72906

Comment 21

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Make send_transaction take &mut self. r=gw

Comment 22

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Renamed clone_sender into create_sender. r=gw

This was split off from https://phabricator.services.mozilla.com/D71992

Depends on D74293

Comment 23

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Add BlobImageHandler::create_similar. r=gw

Depends on D74296

Comment 24

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Move some types to webrender_api. r=gw

Depends on D74297

Comment 25

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Use a single shared font instance map in various place. r=gw

Depends on D74298

Comment 26

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Simplify the SetDisplayList message. r=gw

Depends on D74299

Comment 27

[Nicolas Silva [:nical]]

Attachment: Bug 1612440 - Move blob and font resources to the RenderApi. r=gw

Depends on D74300

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c48d5a53322b
Remove ApiMsg::UpdateResources. r=gw
https://hg.mozilla.org/integration/autoland/rev/0b8fcf1dfe42
Make send_transaction take &mut self. r=gw
https://hg.mozilla.org/integration/autoland/rev/a9a32798b174
Renamed clone_sender into create_sender. r=gw
https://hg.mozilla.org/integration/autoland/rev/30b5173170a4
Add BlobImageHandler::create_similar. r=gw

Comment 29

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c48d5a53322b
https://hg.mozilla.org/mozilla-central/rev/0b8fcf1dfe42
https://hg.mozilla.org/mozilla-central/rev/a9a32798b174
https://hg.mozilla.org/mozilla-central/rev/30b5173170a4

Comment 30

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c48d5a53322b
https://hg.mozilla.org/mozilla-central/rev/0b8fcf1dfe42
https://hg.mozilla.org/mozilla-central/rev/a9a32798b174
https://hg.mozilla.org/mozilla-central/rev/30b5173170a4

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0794b8100c6c
Forward glyph dimension and index requests through the scene builder thread. r=gw
https://hg.mozilla.org/integration/autoland/rev/871a79b86792
Simplify the SetDisplayList message. r=gw
https://hg.mozilla.org/integration/autoland/rev/eb730f48c250
Move some types to webrender_api. r=gw

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60280b67c95d
Use a single shared font instance map in various place. r=gw

Comment 33

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0794b8100c6c
https://hg.mozilla.org/mozilla-central/rev/871a79b86792
https://hg.mozilla.org/mozilla-central/rev/eb730f48c250

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9eaeeec4148
Move blob and font resources to the RenderApi. r=gw

Comment 35

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/60280b67c95d
https://hg.mozilla.org/mozilla-central/rev/a9eaeeec4148

Comment 36

[Florin Strugariu [:Bebe]]

== Change summary for alert #26001 (as of Wed, 20 May 2020 03:49:19 GMT) ==

Improvements:

3% raptor-tp6-yahoo-mail-firefox-cold fcp linux64-shippable-qr opt 334.50 -> 324.75

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26001

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8990e5bd9960
Merge the webrender::Transaction and webrender_api::TransactionMsg structs. r=gw
https://hg.mozilla.org/integration/autoland/rev/1981a4af9b8e
Separate SceneBuilderRequest in two. r=gw,aosmond

Comment 38

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8990e5bd9960
https://hg.mozilla.org/mozilla-central/rev/1981a4af9b8e

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07e83df29896
Send some messages directly to the scene builder thread. r=gw

Comment 40

[Nicolas Silva [:nical]]

oops.

Comment 41

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/07e83df29896

Comment 42

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== Change summary for alert #26927 (as of Mon, 14 Sep 2020 10:58:35 GMT) ==

Improvements:

8% displaylist_mutate windows10-64-shippable-qr opt e10s stylo 2,785.92 -> 2,559.58
6% displaylist_mutate windows10-64-shippable-qr opt e10s stylo 2,747.33 -> 2,574.94
4% displaylist_mutate linux64-shippable-qr opt e10s stylo 2,962.74 -> 2,854.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26927