Add external event notifications in transactions. #3020

nical · 2018-09-05T12:25:13Z

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

This change is

bors-servo · 2018-09-05T17:19:03Z

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

kvark · 2018-09-05T17:58:01Z

webrender/src/resource_cache.rs

-            match update {
+        drain_filter(
+            updates,
+            |update: &mut ResourceUpdate| match *update {


isn't the update type inferred from the closure argument type? we shouldn't need to specify : &mut ResourceUpdate, unless you want it explicitly

I thought so too, then I was battling some build errors and ended up putting that and thought it was what helped. Turns out this wasn't what fixed my trouble since it indeed compiles when I remove the argument type now.

kvark · 2018-09-05T18:16:57Z

webrender/src/util.rs

    device_rect.round_out()
 }
+
+/// Run a first callback over all elements in the array. If the callback returns true,


nit: "the first callback"

kvark · 2018-09-05T18:17:09Z

webrender/src/util.rs

 }
+
+/// Run a first callback over all elements in the array. If the callback returns true,
+/// The element is removed from the array and moved to a second callback.


nit: "the element"

kvark · 2018-09-05T18:52:10Z

webrender/src/util.rs

+/// When that happens, code like:
+///
+/// ```
+/// let filter = |&mut op| {


can we have that in unit tests instead of (or in addition to) the documentation?

I don't think you can even write |&mut op| so checking these by the compiler would help ;)

kvark · 2018-09-05T18:54:22Z

webrender/src/util.rs

+/// ```
+///
+/// See https://doc.rust-lang.org/std/vec/struct.Vec.html#method.drain_filter
+pub fn drain_filter<T, F1, F2>(


nit: can we rename F1 -> Filter and F2 -> Action?

kvark · 2018-09-05T18:57:18Z

webrender/src/util.rs

+    let mut i = 0;
+    while i != vec.len() {
+        if filter(&mut vec[i]) {
+            action(vec.remove(i));


This is quadratic asymptotic complexity, or rather O(N*M) where M is the number of filter matches. Can we do better?

We can but should we? We'll get the the optimized version for free when the std implementation stabilizes and I am certain this will not show up in profiles.
To be honest it also rubs me the wrong way and I'd probably have made more of an effort if we weren't getting the better implementation through std eventually.

ok, that's fine by me, assuming there is a limited array size on the input

Right now the size of resource updates vector is 0 the vast majority of times, 3 while playing a video. occasionally 1 when loading an image, etc.

kvark · 2018-09-05T19:02:04Z

webrender_api/src/api.rs

        self.merge(resources);
    }

+    pub fn notify(&mut self, event: ExternalEvent) {


can we have some documentation here on what does this mean? When will the notification be fired, and why we need it.

kvark · 2018-09-06T13:52:39Z

webrender_api/src/api.rs

+    // potentially long blob rasterization or scene build is ready to be rendered.
+    // sp that the tab-switching integration can react adequately when tab
+    // switching takes too long. For this use case what matters is that the
+    // notification doesn't fire before scene building.


it's not clear from the description if it fires after blob rasterization or after the scene building

kvark · 2018-09-06T13:52:41Z

webrender_api/src/api.rs


+    // Note: Gecko uses this to get notified when a transaction that contains
+    // potentially long blob rasterization or scene build is ready to be rendered.
+    // sp that the tab-switching integration can react adequately when tab


kvark · 2018-09-10T18:30:55Z

webrender/src/render_backend.rs

-        }
+        drain_filter(
+            &mut notifications,
+            |n| { n.when() == CheckPoint::AfterFrameBuilding },


nit: CheckPoint should be a single word

kvark · 2018-09-10T18:31:57Z

webrender_api/src/api.rs

+
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
+pub enum CheckPoint {
+    AfterSceneBuilding,


Maybe SceneBuilt and FrameBuilt?

bors-servo · 2018-09-10T23:39:24Z

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

nical · 2018-09-12T13:04:51Z

@kvark are we good to go?

kvark · 2018-09-12T14:51:49Z

@nical yes, if you are confident that a try push isn't needed here :)

nical · 2018-09-12T14:54:25Z

@bors-servo r=kvark

bors-servo · 2018-09-12T14:54:27Z

📌 Commit 503be4a has been approved by kvark

bors-servo · 2018-09-12T14:54:31Z

⌛ Testing commit 503be4a with merge fb61f1b...

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  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3020)

bors-servo · 2018-09-12T16:14:22Z

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

kvark reviewed Sep 5, 2018

View reviewed changes

nical force-pushed the txn-notif branch 3 times, most recently from e626c5f to 11db1a8 Compare September 5, 2018 21:50

kvark reviewed Sep 6, 2018

View reviewed changes

nical force-pushed the txn-notif branch 2 times, most recently from 2034b12 to 2fa053c Compare September 6, 2018 15:42

kvark reviewed Sep 10, 2018

View reviewed changes

nical force-pushed the txn-notif branch from 1e2955e to 6670f59 Compare September 10, 2018 22:09

nical added 3 commits September 11, 2018 09:35

Add util::drain_filter.

3a5c872

Add external event notifications in transactions.

0e018f1

Use a trait object to handle notifications.

503be4a

nical force-pushed the txn-notif branch from 6670f59 to 503be4a Compare September 11, 2018 07:39

bors-servo merged commit 503be4a into servo:master Sep 12, 2018

Add external event notifications in transactions. #3020

Add external event notifications in transactions. #3020

Uh oh!

Conversation

nical commented Sep 5, 2018 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors-servo commented Sep 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors-servo commented Sep 10, 2018

Uh oh!

nical commented Sep 12, 2018

Uh oh!

kvark commented Sep 12, 2018

Uh oh!

nical commented Sep 12, 2018

Uh oh!

bors-servo commented Sep 12, 2018

Uh oh!

bors-servo commented Sep 12, 2018

Uh oh!

bors-servo commented Sep 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nical commented Sep 5, 2018 •

edited by larsbergstrom

Loading