New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a low priority scene builder thread. #2989

Merged
merged 3 commits into from Sep 5, 2018

Conversation

Projects
None yet
3 participants
@nical
Collaborator

nical commented Aug 27, 2018

This goes on top Kats' work from bug 1477819 and replaces the pair of rasterizer thread by a single low-priority scene builder thread which coordinates with the normal scene builder thread.

The low priority scene builder is an additional thread that can be used to perform some work before forwarding the transaction to the "normal" scene builder thread.
This lets us choose to rasterize very expensive content blobs without blocking the scene builder allowing updates of the UI display list.
Since all low priority scenes request get forwarded to the regular scene builder as well, any operation that affects both threads (such as flushes or shutting down) can simply be sent to the low priority thread.

The flow between threads looks like this:

+------------------+     +------------------+
| LowPSceneBuilder | --> |   SceneBuilder   |
+------------------+     +------------------+
              ^           ^     |
              |           |     |
              |           |     v
         +-----------------------+      +-----------+
api ---> |     RenderBackend     | ---> |  Renderer |
         +-----------------------+      +-----------+


This change is Reviewable

@kvark

So the idea here is that LP scene builder has the same API as the regular one, but it only cares about blobs and then forwards the request to build the scene to the real (high priority) builder?

Show outdated Hide outdated webrender/src/scene_builder.rs
@nical

This comment has been minimized.

Show comment
Hide comment
@nical

nical Aug 28, 2018

Collaborator

So the idea here is that LP scene builder has the same API as the regular one, but it only cares about blobs and then forwards the request to build the scene to the real (high priority) builder?

Yes. If there are other expensive operations that we'd want to split out of the regular scene building we can (and should) make them happen in the low-priority scene builder as well, but in the short term I need large content blobs to not block the UI.

The low priority queue is opt-in, so for widgets that don't contains web pages the low and high priority senders feed the same queue (which means no-reordering of transactions).

Collaborator

nical commented Aug 28, 2018

So the idea here is that LP scene builder has the same API as the regular one, but it only cares about blobs and then forwards the request to build the scene to the real (high priority) builder?

Yes. If there are other expensive operations that we'd want to split out of the regular scene building we can (and should) make them happen in the low-priority scene builder as well, but in the short term I need large content blobs to not block the UI.

The low priority queue is opt-in, so for widgets that don't contains web pages the low and high priority senders feed the same queue (which means no-reordering of transactions).

bors-servo added a commit that referenced this pull request Sep 4, 2018

Auto merge of #2998 - nical:scene-thread, r=kvark
Move the pending scene data structure to the SceneBuilder thread

This PR reorganizes the code in the render backend and scene builder such that:

 - The render backend now only manages a single scene per document: the one it can use for frame building.
 - The scene builder now manages a scene per document and uses it to generate the `FrameBuilder`.
 - The render backend sends to the scene builder thread the events it needs to keep the Scene data structure up to date.
 - Transactions have been consolidated around 3 structs: `TransactionMsg` is what the API sends to the render backend, `Transaction` is what the render backend creates and sends to the scene builder thread(s), and `BuiltTransaction` is what the scene builder sends back to the render backend after scene building. These three structs represent the same thing but at different stages of the pipeline.
 - A *lot* of code was simplified (for example no more calling `update_document` twice with different parameters before and after scene building to have it do different things)

This change is needed in order to get the low priority scene building in #2989 to work without requiring extra handshakes between the scene builder and the render backend threads.

This is rebased on top of #2992.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2998)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 4, 2018

Contributor

☔️ The latest upstream changes (presumably #2998) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors-servo commented Sep 4, 2018

☔️ The latest upstream changes (presumably #2998) made this pull request unmergeable. Please resolve the merge conflicts.

Add a low priority scene builder thread.
The low priority scene builder is an additional thread that can be used to perform some work before forwarding the transaction to the "normal" scene builder thread.
This lets us choose to rasterize very expensive content blobs without blocking the scene builder allowing updates of the UI disply list.
Since all low priority scenes request get forwarded to the regular scene builder as well, any operation that affects both threads (such as flushes or shutting down) can simply be sent to the low priority thread.

@nical nical changed the title from (WIP) Add a low priority scene builder thread. to Add a low priority scene builder thread. Sep 5, 2018

nical added some commits Aug 28, 2018

Make low priority transaction support optional.
This avoids the cost of an extra thread per renderer when we don't need it.
If support is disabled for a given renderer, low and high preirity transactions go to the same queue.
@nical

This comment has been minimized.

Show comment
Hide comment
@nical

nical Sep 5, 2018

Collaborator

r? @kvark

Collaborator

nical commented Sep 5, 2018

r? @kvark

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark

kvark Sep 5, 2018

Member

Frankly speaking, I'm not a fan of ad-hoc threading model that the PR extends. I think we should eventually have a more flexible job scheduling system that avoids blocking and respects dependencies. So I suppose this is an intermediate solution for us to ship something.
@bors-servo r+

Member

kvark commented Sep 5, 2018

Frankly speaking, I'm not a fan of ad-hoc threading model that the PR extends. I think we should eventually have a more flexible job scheduling system that avoids blocking and respects dependencies. So I suppose this is an intermediate solution for us to ship something.
@bors-servo r+

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 5, 2018

Contributor

📌 Commit 1f03603 has been approved by kvark

Contributor

bors-servo commented Sep 5, 2018

📌 Commit 1f03603 has been approved by kvark

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 5, 2018

Contributor

⌛️ Testing commit 1f03603 with merge 46af5bf...

Contributor

bors-servo commented Sep 5, 2018

⌛️ Testing commit 1f03603 with merge 46af5bf...

bors-servo added a commit that referenced this pull request Sep 5, 2018

Auto merge of #2989 - nical:low-prio-raster, r=kvark
Add a low priority scene builder thread.

This goes on top Kats' work from [bug 1477819](https://bugzilla.mozilla.org/show_bug.cgi?id=1477819) and replaces the pair of rasterizer thread by a single low-priority scene builder thread which coordinates with the normal scene builder thread.

The low priority scene builder is an additional thread that can be used to perform some work before forwarding the transaction to the "normal" scene builder thread.
This lets us choose to rasterize very expensive content blobs without blocking the scene builder allowing updates of the UI display list.
Since all low priority scenes request get forwarded to the regular scene builder as well, any operation that affects both threads (such as flushes or shutting down) can simply be sent to the low priority thread.

The flow between threads looks like this:

```
+------------------+     +------------------+
| LowPSceneBuilder | --> |   SceneBuilder   |
+------------------+     +------------------+
              ^           ^     |
              |           |     |
              |           |     v
         +-----------------------+      +-----------+
api ---> |     RenderBackend     | ---> |  Renderer |
         +-----------------------+      +-----------+

```

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2989)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 5, 2018

Contributor

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 46af5bf to master...

Contributor

bors-servo commented Sep 5, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 46af5bf to master...

@bors-servo bors-servo merged commit 1f03603 into servo:master Sep 5, 2018

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@nical nical deleted the nical:low-prio-raster branch Sep 5, 2018

@nical

This comment has been minimized.

Show comment
Hide comment
@nical

nical Sep 5, 2018

Collaborator

I think we should eventually have a more flexible job scheduling system that avoids blocking and respects dependencies.

I've been thinking about things like this a lot lately. There is definitely things we can do to model the flow and ordering of things using data instead of implicitly through threads control flow and I'd also like a more graph-like scheduling system.
That said I think that it'll take time to get something like that right, and also I think'd it'd be hard to nail the requirements at the first attempt without having brought webrender to a shippable state (we figured out we needed this extra level of asynchronicity just a few weeks ago).

It'll be interesting whenever we get to that.

Collaborator

nical commented Sep 5, 2018

I think we should eventually have a more flexible job scheduling system that avoids blocking and respects dependencies.

I've been thinking about things like this a lot lately. There is definitely things we can do to model the flow and ordering of things using data instead of implicitly through threads control flow and I'd also like a more graph-like scheduling system.
That said I think that it'll take time to get something like that right, and also I think'd it'd be hard to nail the requirements at the first attempt without having brought webrender to a shippable state (we figured out we needed this extra level of asynchronicity just a few weeks ago).

It'll be interesting whenever we get to that.

bors-servo added a commit that referenced this pull request Sep 12, 2018

Auto merge of #3020 - nical:txn-notif, r=kvark
Add external event notifications in transactions.

This allows gecko to attach a callback to the transaction and run it after the scene is built to notify the tab switcher that the work is done.

The first commit adds a drain_filter utility modeled after currently unstable Vec::drain_filter. The idea is to be able to get a vector of operations, and each stage of the pipeline can decide to move operations out of the the array and handle them or let them for the next stages to consume.
Hopefully we can use this to merge back the frame_ops, scene_ops and the other vectors in the `Transaction` struct.
This was previously inconvenient because of how some operations need to move elements out of the vector but while we need to keep the array alive at the same time, and drain_filter makes that less painful.

This PR is based on top of #2989

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3020)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment