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

Move the pending scene data structure to the SceneBuilder thread #2998

Merged
merged 7 commits into from Sep 4, 2018

Conversation

Projects
None yet
4 participants
@nical
Collaborator

nical commented Aug 30, 2018

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.


This change is Reviewable

@nical nical changed the title from Scene thread to Move the pending scene data structure to the SceneBuilder thread Aug 30, 2018

@nical

This comment has been minimized.

Show comment
Hide comment
@nical

nical Aug 30, 2018

Collaborator

I am so sorry for this wall of code. Most of it is moving things around. I think that the code is quite a bit easier to understand after this change than before but the diff view doesn't do it justice. You can ignore the first 5 commits which are from #2992.

Collaborator

nical commented Aug 30, 2018

I am so sorry for this wall of code. Most of it is moving things around. I think that the code is quite a bit easier to understand after this change than before but the diff view doesn't do it justice. You can ignore the first 5 commits which are from #2992.

use time::precise_time_ns;
/// Represents the work associated to a transaction before scene building.
pub struct Transaction {

This comment has been minimized.

@nical

nical Aug 30, 2018

Collaborator

Whether splitting the data in Transaction into a lot of vectors the best is up for debate, here is a bit of context:
I realize that having a bunch of small vectors means a bunch of small allocations, however:

  • This representation made it easy to do a few of things in this PR such as knowing whether the transaction needs triggers a scene build and whether it can skip the scene builder by simply looking at which vectors are empty.
  • It will simplify the problem of merging transaction whenever we get to implementing that. Right now if scene building takes a long time, requests can accumulate faster than they are processed which leads to growing delay between user input and rendering which is awful. I want to be able to take several transactions and merge them into one that accumulates the important states while discarding redundant or out of date changes. This is a easier to do if things are organized this way than with a single vector.
@nical

nical Aug 30, 2018

Collaborator

Whether splitting the data in Transaction into a lot of vectors the best is up for debate, here is a bit of context:
I realize that having a bunch of small vectors means a bunch of small allocations, however:

  • This representation made it easy to do a few of things in this PR such as knowing whether the transaction needs triggers a scene build and whether it can skip the scene builder by simply looking at which vectors are empty.
  • It will simplify the problem of merging transaction whenever we get to implementing that. Right now if scene building takes a long time, requests can accumulate faster than they are processed which leads to growing delay between user input and rendering which is awful. I want to be able to take several transactions and merge them into one that accumulates the important states while discarding redundant or out of date changes. This is a easier to do if things are organized this way than with a single vector.

nical added some commits Aug 29, 2018

Move the pending scene data structure to the scene builder thread.
With this change the render backend only sees the state of the scene
that it can use for frame building and the scene builder thread manages
its own scene.
This will allow us to perform low priority scene building and rasterization
asynchronously without having to involve the render backend thread.
Remove the per-document FrameBuilderConfig.
All documents currently always have the same config.
Fix confusion with generate frame/render/composite terminology.
From now on, generate frame means having the frame builder generate a frame (but not necessarily render it), and render means asking the renderer to render a frame.
@nical

This comment has been minimized.

Show comment
Hide comment
@nical

nical Aug 31, 2018

Collaborator

I added a commit that simplifies the terminology around generating frames and rendering (and fixes a bug I caused because of the associated confusion)

Before this PR:

  • render meant building a frame with the frame builder but not necessarily asking the renderer to render it.
  • composite meant asking the renderer to render.

After this PR:

  • generate_frame means asking the frame builder to build a frame.
  • render means asking the renderer to render a frame.
Collaborator

nical commented Aug 31, 2018

I added a commit that simplifies the terminology around generating frames and rendering (and fixes a bug I caused because of the associated confusion)

Before this PR:

  • render meant building a frame with the frame builder but not necessarily asking the renderer to render it.
  • composite meant asking the renderer to render.

After this PR:

  • generate_frame means asking the frame builder to build a frame.
  • render means asking the renderer to render a frame.
@nical

This comment has been minimized.

Show comment
Hide comment
@nical

nical Aug 31, 2018

Collaborator

try push looks good so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e606386b9e899fcde16d8344fdc8c5ddaaf9e6e3

The push has all of the commits since the last WR update so I can't take credit for the unexpected passes. There are also a few failures with rendering differences within fuzzing range and I don't think these can be the result of this PR.

Collaborator

nical commented Aug 31, 2018

try push looks good so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e606386b9e899fcde16d8344fdc8c5ddaaf9e6e3

The push has all of the commits since the last WR update so I can't take credit for the unexpected passes. There are also a few failures with rendering differences within fuzzing range and I don't think these can be the result of this PR.

@kvark

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @kvark and @nical)


webrender/src/render_backend.rs, line 332 at r3 (raw file):

struct DocumentOps {
    scroll: bool,
    generate_frame: bool,

I'd marginally prefer us to be consistent with the rest of the internal code and call this build_frame (and possibly rename the GenerateFrame message as well).


webrender/src/render_backend.rs, line 333 at r3 (raw file):

    scroll: bool,
    generate_frame: bool,
    render: bool,

uuh, so we have an old flag with an entirely new semantics now
would be safer to rename it as well, e.g. show_frame to avoid confusion


webrender/src/render_backend.rs, line 529 at r3 (raw file):

                profile_scope!("SetRootPipeline");

                txn.set_root_pipeline = Some(pipeline_id);

I am slightly confused here at a high level: we receive a scene message for updating the document, but then some of the updates affect Transaction and others affect Document itself. Seems like a recipe for synchronization issues?


webrender/src/render_backend.rs, line 594 at r3 (raw file):

                            replace(&mut txn.rasterized_blobs, Vec::new())
                        );
                        if let Some(rasterizer) = txn.blob_rasterizer.take() {

why are we taking out the blob rasterizer?


webrender/src/render_backend.rs, line 828 at r3 (raw file):

        profile_counters: &mut BackendProfileCounters,
    ) {
        let mut txn = Box::new(Transaction {

why are we boxing the transaction?


webrender/src/render_backend.rs, line 940 at r3 (raw file):

        // rebuild the hit-tester, so we trigger a frame generation
        // step.
        generate_frame |= has_built_scene;

we might need a separate flag for hit tester info update, given that it shouldn't need the full frame build


webrender/src/scene.rs, line 193 at r3 (raw file):

    pub fn has_root_pipeline(&self) -> bool {
        if let Some(ref root_id) = self.root_pipeline_id {

nit: this could look cleaner without a return


webrender/src/scene_builder.rs, line 22 at r2 (raw file):

Previously, nical (Nicolas Silva) wrote…

Whether splitting the data in Transaction into a lot of vectors the best is up for debate, here is a bit of context:
I realize that having a bunch of small vectors means a bunch of small allocations, however:

  • This representation made it easy to do a few of things in this PR such as knowing whether the transaction needs triggers a scene build and whether it can skip the scene builder by simply looking at which vectors are empty.
  • It will simplify the problem of merging transaction whenever we get to implementing that. Right now if scene building takes a long time, requests can accumulate faster than they are processed which leads to growing delay between user input and rendering which is awful. I want to be able to take several transactions and merge them into one that accumulates the important states while discarding redundant or out of date changes. This is a easier to do if things are organized this way than with a single vector.

Does gecko has facilities implemented/planned for accelerating those heap allocations? We have accumulated quite a big technical debt in performance, partly because of those allocations, and I want to see the light at the end of this tunnel.

@nical

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @nical)


webrender/src/render_backend.rs, line 332 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I'd marginally prefer us to be consistent with the rest of the internal code and call this build_frame (and possibly rename the GenerateFrame message as well).

Sounds good to me. I'd like to avoid touching the public API in this PR but I'll rename some of the places now.


webrender/src/render_backend.rs, line 333 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

uuh, so we have an old flag with an entirely new semantics now
would be safer to rename it as well, e.g. show_frame to avoid confusion

I like render because it is about asking the Renderer to do its job so it's really the most unambiguous terminology.


webrender/src/render_backend.rs, line 529 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I am slightly confused here at a high level: we receive a scene message for updating the document, but then some of the updates affect Transaction and others affect Document itself. Seems like a recipe for synchronization issues?

The transaction itself is just info about updating documents. Because of how these things are pipelined document updates that are not applied right away upon receiving the transaction go in this transaction object.


webrender/src/render_backend.rs, line 594 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why are we taking out the blob rasterizer?

Oops, that's a mistake.


webrender/src/render_backend.rs, line 828 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why are we boxing the transaction?

Large structures that move around a lot like this one tend to generate atrocious code behind the scenes, I think that it's a healthy habit to box them when they are not created often (once per transaction isn't often).


webrender/src/render_backend.rs, line 940 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we might need a separate flag for hit tester info update, given that it shouldn't need the full frame build

Agreed. it'll be easy to add the flag when we implement watever's needed to generate only the hit testing info.


webrender/src/scene_builder.rs, line 22 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Does gecko has facilities implemented/planned for accelerating those heap allocations? We have accumulated quite a big technical debt in performance, partly because of those allocations, and I want to see the light at the end of this tunnel.

I don't think there is anything specific in Gecko for this. I believe that our biggest problem with allocations right now in webrender is reallocating/growing large vectors, so these small vectors śhould be anecdotal in comparison.

More generally, I don't have a very strong attachment to the way the Transaction struct is organized (beyond that I want to make sure we can efficiently merge transactions one way or another). I did it this way because it was the simplest thing I could come up with for this PR, but if we want to do something better/more ambitious I'm game (preferably separately from this already big and risky changeset).

@kvark

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kvark and @nical)


webrender/src/render_backend.rs, line 333 at r3 (raw file):

Previously, nical (Nicolas Silva) wrote…

I like render because it is about asking the Renderer to do its job so it's really the most unambiguous terminology.

I'm ok with ending up with render but as an intermediate step name it something other than the old flag


webrender/src/render_backend.rs, line 529 at r3 (raw file):

Previously, nical (Nicolas Silva) wrote…

The transaction itself is just info about updating documents. Because of how these things are pipelined document updates that are not applied right away upon receiving the transaction go in this transaction object.

so on the high level - why not make all mutation of the document to go through a transaction?


webrender/src/render_backend.rs, line 594 at r3 (raw file):

Previously, nical (Nicolas Silva) wrote…

Oops, that's a mistake.

heads up - still not addressed?


webrender/src/render_backend.rs, line 828 at r3 (raw file):

Previously, nical (Nicolas Silva) wrote…

Large structures that move around a lot like this one tend to generate atrocious code behind the scenes, I think that it's a healthy habit to box them when they are not created often (once per transaction isn't often).

How large of a structure are we talking about? Would be great to make this more systematic and document in guidelines, e.g. structures larger than 128 bytes should be boxed.


webrender/src/scene_builder.rs, line 22 at r2 (raw file):

Previously, nical (Nicolas Silva) wrote…

I don't think there is anything specific in Gecko for this. I believe that our biggest problem with allocations right now in webrender is reallocating/growing large vectors, so these small vectors śhould be anecdotal in comparison.

More generally, I don't have a very strong attachment to the way the Transaction struct is organized (beyond that I want to make sure we can efficiently merge transactions one way or another). I did it this way because it was the simplest thing I could come up with for this PR, but if we want to do something better/more ambitious I'm game (preferably separately from this already big and risky changeset).

That's an interesting observation.

nical added some commits Sep 4, 2018

Rename Transaction's render into render_frame.
This is to avoid confusion with the previous meaning of render which was about building the frame rather than rendering it.
@nical

This comment has been minimized.

Show comment
Hide comment
@nical

nical Sep 4, 2018

Collaborator

so on the high level - why not make all mutation of the document to go through a transaction?

The way I picture things in my mind, they all do. There are 3 structs that represent the transaction at different stages of the pipeline TransactionMsg -> Transaction -> BuiltTransaction

Some things get applied between TransactionMsg and Transaction (that's the ones you are referring to in this comment), others between Transaction and BuiltTransaction (scene building, blob rasterization), others after BuiltTransaction (scrolling, frame building for example).

heads up - still not addressed?

Sorry I got mixed up, it was actually not a mistake. It's correct to move the blob rasterizer out of the transaction here because we want to move it into the render backend (it is uniquely owned). Using take() here helped getting things to build because, probably something about partially moving out a boxed struct making the compiler unhappy.

Collaborator

nical commented Sep 4, 2018

so on the high level - why not make all mutation of the document to go through a transaction?

The way I picture things in my mind, they all do. There are 3 structs that represent the transaction at different stages of the pipeline TransactionMsg -> Transaction -> BuiltTransaction

Some things get applied between TransactionMsg and Transaction (that's the ones you are referring to in this comment), others between Transaction and BuiltTransaction (scene building, blob rasterization), others after BuiltTransaction (scrolling, frame building for example).

heads up - still not addressed?

Sorry I got mixed up, it was actually not a mistake. It's correct to move the blob rasterizer out of the transaction here because we want to move it into the render backend (it is uniquely owned). Using take() here helped getting things to build because, probably something about partially moving out a boxed struct making the compiler unhappy.

@kvark

:lgtm:

Reviewed 2 of 2 files at r5.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @nical)

@kvark

kvark approved these changes Sep 4, 2018

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark
Member

kvark commented Sep 4, 2018

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 4, 2018

Contributor

📌 Commit 32bbbe8 has been approved by kvark

Contributor

bors-servo commented Sep 4, 2018

📌 Commit 32bbbe8 has been approved by kvark

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 4, 2018

Contributor

⌛️ Testing commit 32bbbe8 with merge d2c7c36...

Contributor

bors-servo commented Sep 4, 2018

⌛️ Testing commit 32bbbe8 with merge d2c7c36...

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

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

Contributor

bors-servo commented Sep 4, 2018

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

@bors-servo bors-servo merged commit 32bbbe8 into servo:master Sep 4, 2018

3 of 4 checks passed

code-review/reviewable 5 discussions left (nical)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@sotaroikeda

This comment has been minimized.

Show comment
Hide comment
@sotaroikeda

sotaroikeda Sep 10, 2018

Contributor

This seemed to cause regressions of Bug 1489471 and Bug 1489189.

Contributor

sotaroikeda commented Sep 10, 2018

This seemed to cause regressions of Bug 1489471 and Bug 1489189.

@nical nical deleted the nical:scene-thread branch Sep 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment