Make the API transaction-based #1963

nical · 2017-10-30T17:56:03Z

The idea is to replace individual DocumentMsgs in the api messages by vectors of DocumentMsg to provide a way to express the constraint that several DocumentMsg must be handled atomically without any other message interleaving in between.
This also lets us avoid redundant work when we build transactions that contain several operations that would cause the frame and/or scene to be rebuilt.

At the API level, users create a Transaction object and accumulate commands like set_display_list, scroll or update_resources into it and only touch the RenderApi at the end when calling send_transaction.

This change is

nical · 2017-10-30T17:59:21Z

This is a work in progress, it's not too far from being ready but there are still a few unresolved TODOs in the code. For now I haven't removed the RenderApi methods that are in Transaction, so that we can hopefully first land this, update gecko and servo and then remove the deprecated APIs. Hopefully that should decouple the task of updating WebRender and moving to the new API.
Feedback very welcome.

cc @kvark @glennw

glennw · 2017-10-31T01:43:23Z

I haven't looked at this in super detail, but the general patch looks good - the bits I looked at so far make sense to me.

nical · 2017-11-06T15:07:48Z

Gecko try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9224869560da7e7c60097b52ba966c0c8c3a2195

The PR is in a good state to be reviewed now.

kvark · 2017-11-06T17:22:50Z

webrender/src/render_backend.rs

 /// The unique id for WR resource identification.
 static NEXT_NAMESPACE_ID: AtomicUsize = ATOMIC_USIZE_INIT;

+struct DocumentOp {


should be renamed to DocumentOps now

kvark · 2017-11-06T17:22:50Z

webrender/src/render_backend.rs

@@ -422,23 +444,22 @@ impl RenderBackend {
                //           rebuild of the frame!
                if let Some(property_bindings) = property_bindings {
                    doc.scene.properties.set_properties(property_bindings);
-                    doc.build_scene(&mut self.resource_cache);
+                    op.build = true;


we should probably do the same for scroll & render events, instead of having and_render boolean

kvark · 2017-11-06T17:22:50Z

webrender/src/render_backend.rs

+            self.result_tx.send(msg).unwrap();
+            profile_counters.reset();
+
+            if !op.scroll {


Yeah, the whole RenderNotifier trait is kinda arbitrary and modelled around the previous control flow. It should really be a single method with parameters saying how much has changed (scroll, scene, frame, etc.).
I was trying to do this PR without making it a breaking change to make the wr update easier for once (and come back with a breaking cleanup once the APIs are not used anymore). But it'd be good to simplify the RenderNotifier API and avoid this kind of dance.

kvark · 2017-11-06T17:22:50Z

webrender_api/src/api.rs

+///
+/// This mechanism ensures that:
+///  - no other message can be interleaved between two commands that need to be applied together.
+///  - no redundant work is performed if two commands in the same tranasction cause the scen or


"scen", "tranasction"

bors-servo · 2017-11-07T07:20:50Z

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

glennw · 2017-11-12T20:40:47Z

Needs a rebase.

glennw · 2017-11-23T02:33:37Z

@nical Are you still working on this?

nical · 2017-11-23T08:54:49Z

I do plan to finish this but unless there we identify cases that we care about for Austin where this can help performance, it is not in the list of high priority items until the workweek.

glennw · 2017-11-28T05:51:26Z

Closing for now - let's re-open once we're ready to resume work on this.

kvark · 2017-11-28T15:58:16Z

@glennw I don't see much value in closing the issue if it's still to be worked on. Our goal for now is to have performance up, not close as many issues as we can.

nical · 2018-01-03T14:53:25Z

Resuming work on this (rebase in progress).

kvark · 2018-01-03T17:56:08Z

looking good so far 👍

Reviewed 7 of 7 files at r2.
Review status: 6 of 7 files reviewed at latest revision, 4 unresolved discussions.

webrender/src/render_backend.rs, line 488 at r2 (raw file):

                    op.render = true;
                } else {
                    // TODO(nical) - Eeeeeek! The previous code did the equivalent of this, but it doesn't

we need to clear up that semantics bit, it's inherited from long ago. perhaps, nop-scroll was used like a "wakeup" so now we can remove it since we have explicit wakeup

Comments from Reviewable

nical · 2018-01-03T17:58:15Z

I got the PR to build again but before merging I'd like to revive the gecko side as well and do some testing. I may move the resource updates in a separate array because they will likely need some special handling for the async scene building stuff.

nical · 2018-01-05T17:46:30Z

The PR is in a reviewable state now. There are some more cosmethic changes that would be nice to have (such as making SetDisplayList less of a uber-message, and make epochs truly up to the api user, but that can wait. There might be some reorg inside of the Transaction struct to help with the upcoming async scene building, but that should be doable without changing the API itself.

Next is deciding:

whether or not to keep the old API on the side. Personally I'd rather not but that's open for bikeshed.
whether to keep ResourceUpdateQueue. With a dedicated transaction object we can just merge the two, I don't feel strongly either way.

These can be decided later too.

nical · 2018-01-08T10:16:52Z

r? @kvark @glennw

kvark · 2018-01-08T19:35:34Z

Reviewed 4 of 4 files at r4.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

glennw · 2018-01-08T21:08:04Z

webrender/src/render_backend.rs

-            DocumentMsg::GenerateFrame(property_bindings) => {
-                profile_scope!("GenerateFrame");
+            DocumentMsg::UpdateDynamicProperties(property_bindings) => {
+                // Ideally, when there are property bindings present,


This comment and code is obsolete, as of #2043. We shouldn't need to rebuild the scene when the property bindings change.

glennw · 2018-01-09T20:28:06Z

Looks good to me! I'll delegate r+ and leave it to nical to determine if we need a gecko try run for this change, and merge when ready (unless @kvark has any remaining questions).

@bors-servo delegate+

bors-servo · 2018-01-09T20:28:07Z

✌️ @nical can now approve this pull request


        Make WebRender's API transaction based.


        Remove some legacy nonsense.


        Separate the GenerateFrame and UpdateAnimatedProperties messages.

nical · 2018-01-10T17:19:06Z

I had some test timeouts which ended up me messing up the gecko side integration. Works now!

@bors-servo r+

bors-servo · 2018-01-10T17:19:07Z

📌 Commit 1a53083 has been approved by nical

bors-servo · 2018-01-10T17:19:09Z

⌛ Testing commit 1a53083 with merge 722e8b1...


        Auto merge of #1963 - nical:transaction, r=nical

Make the API transaction-based The idea is to replace individual `DocumentMsg`s in the api messages by vectors of `DocumentMsg` to provide a way to express the constraint that several DocumentMsg must be handled atomically without any other message interleaving in between. This also lets us avoid redundant work when we build transactions that contain several operations that would cause the frame and/or scene to be rebuilt. At the API level, users create a `Transaction` object and accumulate commands like `set_display_list`, `scroll` or `update_resources` into it and only touch the `RenderApi` at the end when calling `send_transaction`.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1963)

staktrace · 2018-01-10T17:48:52Z

@nical Please post the gecko integration patch to bug 1428766 so I can include it in my testing try pushes after this merges. Thanks!

nical · 2018-01-10T17:55:02Z

@staktrace I tried to make this PR non-breaking so that we can (hopefully) land the integration patches after the wr update (or that's the theory/intent anyway).
After we have integrated the new APIs with gecko I'll remove the old APIs here.

staktrace · 2018-01-10T18:10:21Z

Ah cool, that works too :)

bors-servo · 2018-01-10T18:12:42Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nical
Pushing 722e8b1 to master...


        Auto merge of #2298 - nical:txn-ification, r=nical

Move code to the new transaction API. Cleaning up after #1963.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2298)


        Auto merge of #2298 - nical:txn-ification, r=nical

Move code to the new transaction API. Cleaning up after #1963.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2298)

nical force-pushed the nical:transaction branch 2 times, most recently from 242785c to 66b646e Oct 31, 2017

kvark self-requested a review Nov 6, 2017

nical force-pushed the nical:transaction branch from 66b646e to 7356c81 Nov 6, 2017

nical changed the title ~~[WIP] Make the API transaction-based~~ Make the API transaction-based Nov 6, 2017

kvark approved these changes Nov 6, 2017

View changes

nical mentioned this pull request Nov 20, 2017

DocumentAPI support on the Renderer side #2050

Merged

6 of 6 tasks complete

glennw closed this Nov 28, 2017

nical reopened this Jan 3, 2018

nical force-pushed the nical:transaction branch 2 times, most recently from f7e7ba6 to 1f1cfdf Jan 3, 2018

nical force-pushed the nical:transaction branch 2 times, most recently from 19b4c66 to 75cc81d Jan 5, 2018

glennw requested changes Jan 8, 2018

View changes

nical force-pushed the nical:transaction branch from 75cc81d to bc6e315 Jan 9, 2018

glennw approved these changes Jan 9, 2018

View changes

nical added 3 commits Oct 17, 2017

Make WebRender's API transaction based.

cf6c45c

Remove some legacy nonsense.

7d56932

Separate the GenerateFrame and UpdateAnimatedProperties messages.

Loading status checks…

1a53083

nical force-pushed the nical:transaction branch from bc6e315 to 1a53083 Jan 10, 2018

nical deleted the nical:transaction branch Jan 10, 2018

nical mentioned this pull request Jan 12, 2018

Move code to the new transaction API. #2298

Merged

servo / webrender

Make the API transaction-based #1963

Make the API transaction-based #1963

nical commented Oct 30, 2017 •

edited by larsbergstrom

nical commented Oct 30, 2017

glennw commented Oct 31, 2017

nical commented Nov 6, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Nov 7, 2017

glennw commented Nov 12, 2017

glennw commented Nov 23, 2017

nical commented Nov 23, 2017

glennw commented Nov 28, 2017

kvark commented Nov 28, 2017

nical commented Jan 3, 2018

kvark commented Jan 3, 2018

nical commented Jan 3, 2018

nical commented Jan 5, 2018

nical commented Jan 8, 2018

kvark commented Jan 8, 2018

This comment has been minimized.

glennw commented Jan 9, 2018

bors-servo commented Jan 9, 2018

nical commented Jan 10, 2018

bors-servo commented Jan 10, 2018

bors-servo commented Jan 10, 2018

staktrace commented Jan 10, 2018

nical commented Jan 10, 2018

staktrace commented Jan 10, 2018

bors-servo commented Jan 10, 2018

servo / webrender

Join GitHub today

Make the API transaction-based #1963

Make the API transaction-based #1963

Conversation

nical commented Oct 30, 2017 • edited by larsbergstrom

nical commented Oct 30, 2017

glennw commented Oct 31, 2017

nical commented Nov 6, 2017

This comment has been minimized.

kvark Nov 6, 2017

This comment has been minimized.

kvark Nov 6, 2017

This comment has been minimized.

kvark Nov 6, 2017

This comment has been minimized.

nical Nov 6, 2017

This comment has been minimized.

kvark Nov 6, 2017

bors-servo commented Nov 7, 2017

glennw commented Nov 12, 2017

glennw commented Nov 23, 2017

nical commented Nov 23, 2017

glennw commented Nov 28, 2017

kvark commented Nov 28, 2017

nical commented Jan 3, 2018

kvark commented Jan 3, 2018

nical commented Jan 3, 2018

nical commented Jan 5, 2018

nical commented Jan 8, 2018

kvark commented Jan 8, 2018

This comment has been minimized.

glennw Jan 8, 2018

glennw commented Jan 9, 2018

bors-servo commented Jan 9, 2018

nical commented Jan 10, 2018

bors-servo commented Jan 10, 2018

bors-servo commented Jan 10, 2018

staktrace commented Jan 10, 2018

nical commented Jan 10, 2018

staktrace commented Jan 10, 2018

bors-servo commented Jan 10, 2018

nical commented Oct 30, 2017 •

edited by larsbergstrom