Track removed pipelines and expose them in the renderer. #2347

nical · 2018-01-26T17:06:21Z

This information is useful to manage the lifetime of resources that are owned by the embedder and used by the renderer such as external images.

Fixes #2345.

This change is

kvark · 2018-01-26T20:41:22Z

Could we perhaps add a special value Epoch::FINAL, which is getting pushed to the pipeline_epoch_map and then handled after the client calls flush_rendered_epochs?

glennw · 2018-01-29T01:18:33Z

Looks like a compile error on CI - I haven't looked any deeper than that.

nical · 2018-01-29T14:21:06Z

Could we perhaps add a special value Epoch::FINAL, which is getting pushed to the pipeline_epoch_map and then handled after the client calls flush_rendered_epochs?

We can if you feel strongly about it. Personally I prefer keeping the epoch map (which represent the continuous state of epochs whether they change or not, and that you look at when needed) and the removed pipelines (which are more like one-off events, and missing any of them can be a leak) separated because they represent information somewhat differently and the consumer of the API presumably doesn't do exactly the same things with them.

The build error is odd, this code compiles locally and rustc is complaining that the document is not mutable while it is declared mut a few lines above. I wonder if the recent switch to rustc 1.23 fixed it.

nical · 2018-01-30T18:39:46Z

r? @kvark or @glennw

kvark · 2018-01-30T18:43:07Z

I just find this to be too verbose, especially considering that both flushes are very similar (and can be combined as I suggested). Leaving for @glennw to look over.

nical · 2018-01-30T21:58:11Z

We can address the verbosity of the API by storing the pipeline epoch map and the removed pipeline list in a PipelineInfo struct and return it in a single flush_pipeline_info method (that's actually what the gecko side does to pass them around). On the render backend side, maintaining the two structures is really a different logic, so the amount of code wouldn't change anyway, but we can still store and pass the two data structures around in a PipelineInfo struct as well to have a single parameter in a few functions where they take two.

glennw · 2018-01-31T07:20:01Z

I do think it's probably a good idea to keep them as separate concepts. It makes sense to me that the user would potentially care about removed pipelines, but not really care about rendered pipelines, in some cases.

It does seem unfortunate that we effectively leak memory (in tiny amounts) if the user never calls those flush APIs, but that's maybe not a huge deal.

Perhaps we could consider a single flush function that returns the two vectors to reduce the API surface? Or would it make sense for one or both of those vectors to be returned from either update() or render()? That would solve any leaks, since they'll just be freed if the user doesn't look at them. I'm not sure what the semantics would be in the case of render returning an error result though.

Either way, I don't feel strongly about any of the above - we can always iterate and improve on this later as required. So I'll delegate r+ to @nical and leave to work out with @kvark if he feels further changes are needed :)

@bors-servo delegate+

bors-servo · 2018-01-31T07:20:03Z

✌️ @nical can now approve this pull request

nical · 2018-01-31T14:41:38Z

I added a commit that does what I suggested in my previous comment. @kvark let me know if you feel better about this.

It does seem unfortunate that we effectively leak memory (in tiny amounts) if the user never calls those flush APIs, but that's maybe not a huge deal.

Agreed. I didn't feel bad enough about it that I'd add debug logic to track whether we render N frames without ever flushing the removed pipelines, but that would be easy to add. I think that it'd be error prone to just flush every frame unless we return the PipelineInfo in the render function. Whoever feels strongly about this can fix it as a followup, though. There are many more important things that we could be spending our time on.

kvark · 2018-01-31T14:59:14Z

webrender/src/frame.rs

+        RenderedDocument::new(
+            PipelineInfo {
+                epochs: self.pipeline_epoch_map.clone(),
+                removed_pipelines: replace(&mut self.removed_pipelines, Vec::new()),


wouldn't this suffer from the same problem we have with GPU cache, where the consistency of the system depends on strictly producing and consuming rendered documents in order? Instead, I'd like to see the backend being able to produce any number of rendered documents, and Renderer being able to skip any of them, and still be correct. At least the current GPU cache updates are structured this way.

On the render backend side I believe I made sure that ordering is preserved and notifications are not lost no matter which order we build the scenes and frames in (that's the reason why the removed pipelines vector goes through all of the various pieces of the render backend and not just the scene and/or frame context).
On the renderer side, I think that it works as long as all documents render to the same swap chain, in the sense that the goal is to notify removed pipelines at least one frame after the last time it was rendered, so even if we skip rendering a document but notify its removed pipelines, said pipelines have already been removed, won't be around next time we do render the document and can be safely removed, I think.

As far as ordering go when sending to the renderer, I think that there shouldn't be any mismatch because the information is passed along with the frame itself (isn't in an separate channel or something that would need extra synchronization).

So I believe that this should work as intended even if we don't render some documents as long as it's in one swap chain (when we support direct composition and friends we'll need to track this information per swap chain). Unless by skipping rendering a document you mean receive a document in the renderer but keep sending drawing the commands of its previous version, in which case we need to add some extra logic to manage that, probably by tracking this information per document in the renderer.

My desire here is to have rendering independent of modelling. The backend should be able to spew as many frames as it needs, and the renderer should be able to consume any subset of those. This is reflected in the type system by the fact we don't mutate most of the things when producing a RenderedDocument.

I agree with this desire, I don't think this interferes.

Since removed pipelines aren't states that define the scene but really events associated to it that must be sent once, there is no getting around moving them through the pipeline which requires the events to be mutable.

In addition, we need these events to be ordered with frames that are sent to the renderer. By far the easiest way to preserve this ordering is to send the information along with the frames and require that the renderer (whether or not it ends up rendering the frames) at least collects the events and makes them available to the embedder.

To preserve the non-mutable ref to the scene we would need to track the removed pipelines in parallel to the scene instead of inside the scene, make sure that we associate the right list of removed pipelines with the right scene and not forget to send the removed pipelines of a given scene when we produce a frame out of it. That sounds a lot more error prone. If you care a lot about the scene being passed immutably, then I wouldn't be opposed to using a Cell for the removed pipelines vector.

In addition, we need these events to be ordered with frames that are sent to the renderer.

They don't have to be 1:1. We only have to guarantee that a pipeline is not used after the removed pipeline event arrives. I see the situation exactly matching GPU cache updates, and I want the handling to be consistent. Can we have a single mechanism/entity responsible for those one-time updates? Perhaps, by extending and renaming UpdateGpuCache event or something. Just handle it all in the same way.

That sounds a lot more error prone.

This is much easier than you imply. We already have association of the Document - Scene.

kvark · 2018-01-31T14:59:14Z

webrender/examples/common/boilerplate.rs

@@ -314,6 +314,7 @@ pub fn main_wrapper<E: Example>(

        renderer.update();
        renderer.render(framebuffer_size).unwrap();
+        let _ = renderer.flush_pipeline_info();


this is better 👍

kvark · 2018-01-31T14:59:14Z

webrender/src/render_backend.rs

@@ -120,7 +120,7 @@ impl Document {
        // this code is why we have `Option`, which is never `None`
        let frame_builder = self.frame_ctx.create(
            self.frame_builder.take().unwrap(),
-            &self.scene,
+            &mut self.scene,


this is worrying

it's unfortunately needed to flush the removed pipelines out of the scene. Storing them in the scene itself is part of what helps making sure we don't mess up ordering if we modify a scene and render frames from previous versions. I don't think there is much we can do about the mutability here other than things like Cell which I find more worrying.

Perhaps we could keep the list of removed pipelines in the Document itself as opposed to the Scene? There are two spots where remove_pipelines is called, so we aren't going to be doing much extra work, but the semantics of Scene would be better then.

kvark · 2018-01-31T14:59:14Z

webrender/src/renderer.rs

+}
+
+impl PipelineInfo {
+    pub fn new() -> Self {


derive(Default) ftw

Good point, fixed.

nical · 2018-01-31T17:08:52Z

@staktrace heads up this is an API breaking change. Renderer::flush_rendered_epochs is replaced with Renderer::flush_pipeline_info and the return type is slightly different. The patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1432708 will have the corresponding gecko change.

staktrace · 2018-01-31T18:01:52Z

@nical please have that patch reviewed by somebody prior to merging this PR.


        Track removed pipelines and expose them in the renderer.

This information is useful to manage the lifetime of resources that are owned by the embedder and used by the renderer such as external images.


        Group the pipeline epoch map and removed pipelines into a PipelineInf…

…o struct.


        Track removed pipelines in the document instead of the frame context.

nical · 2018-02-07T16:21:49Z

@bors-servo r=glennw

bors-servo · 2018-02-07T16:21:50Z

📌 Commit 8d265ae has been approved by glennw

bors-servo · 2018-02-07T16:21:53Z

⌛ Testing commit 8d265ae with merge 30f2cd5...


        Auto merge of #2347 - nical:removed-pipelines, r=glennw

Track removed pipelines and expose them in the renderer. This information is useful to manage the lifetime of resources that are owned by the embedder and used by the renderer such as external images. Fixes #2345.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2347)

bors-servo · 2018-02-07T17:07:56Z

☀️ Test successful - status-appveyor, status-taskcluster, status-travis
Approved by: glennw
Pushing 30f2cd5 to master...

kvark self-requested a review Jan 26, 2018

nical force-pushed the nical:removed-pipelines branch from b7a3bb5 to ecb15de Jan 30, 2018

kvark reviewed Jan 31, 2018

View changes

nical force-pushed the nical:removed-pipelines branch from 449cbc8 to 75e949c Jan 31, 2018

nical added 2 commits Jan 26, 2018

Track removed pipelines and expose them in the renderer.

ec19429

This information is useful to manage the lifetime of resources that are owned by the embedder and used by the renderer such as external images.

Group the pipeline epoch map and removed pipelines into a PipelineInf…

7056a51

…o struct.

nical force-pushed the nical:removed-pipelines branch from bda5b46 to 3bd5cc4 Feb 7, 2018

Track removed pipelines in the document instead of the frame context.

Loading status checks…

8d265ae

nical force-pushed the nical:removed-pipelines branch from 3bd5cc4 to 8d265ae Feb 7, 2018

bors-servo merged commit 8d265ae into servo:master Feb 7, 2018
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

bors-servo mentioned this pull request Feb 7, 2018

Async scene building. #2362

Merged

6 of 7 tasks complete

nical deleted the nical:removed-pipelines branch Feb 7, 2018

servo / webrender

Track removed pipelines and expose them in the renderer. #2347

Track removed pipelines and expose them in the renderer. #2347

nical commented Jan 26, 2018 •

edited by larsbergstrom

kvark commented Jan 26, 2018

glennw commented Jan 29, 2018

nical commented Jan 29, 2018 •

edited

nical commented Jan 30, 2018

kvark commented Jan 30, 2018

nical commented Jan 30, 2018

glennw commented Jan 31, 2018

bors-servo commented Jan 31, 2018

nical commented Jan 31, 2018

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.

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.

This comment has been minimized.

nical commented Jan 31, 2018

staktrace commented Jan 31, 2018

nical commented Feb 7, 2018

bors-servo commented Feb 7, 2018

bors-servo commented Feb 7, 2018

bors-servo commented Feb 7, 2018

servo / webrender

Join GitHub today

Track removed pipelines and expose them in the renderer. #2347

Track removed pipelines and expose them in the renderer. #2347

Conversation

nical commented Jan 26, 2018 • edited by larsbergstrom

kvark commented Jan 26, 2018

glennw commented Jan 29, 2018

nical commented Jan 29, 2018 • edited

nical commented Jan 30, 2018

kvark commented Jan 30, 2018

nical commented Jan 30, 2018

glennw commented Jan 31, 2018

bors-servo commented Jan 31, 2018

nical commented Jan 31, 2018

This comment has been minimized.

This comment has been minimized.

nical Jan 31, 2018 • edited

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nical commented Jan 31, 2018

staktrace commented Jan 31, 2018

nical commented Feb 7, 2018

bors-servo commented Feb 7, 2018

bors-servo commented Feb 7, 2018

bors-servo commented Feb 7, 2018

nical commented Jan 26, 2018 •

edited by larsbergstrom

nical commented Jan 29, 2018 •

edited

nical Jan 31, 2018 •

edited