Store the built state in the PipelineInfo #2865

staktrace · 2018-07-04T15:40:23Z

This adds a flag to the entries in the PipelineInfo that records whether
or not a pipeline was actually built at a given epoch. It is possible
for pipelines to have display lists that are not built if they are not
connected to the root pipeline via a chain of iframe items, and in some
cases Gecko needs to be able to distinguish between whether a pipeline
was built or not during a particular render.

This change is

staktrace · 2018-07-04T15:44:07Z

This is desired for https://bugzilla.mozilla.org/show_bug.cgi?id=1473352

kvark · 2018-07-05T13:20:41Z

@stacktrace The building is triggered, as you say, by having the pipeline connected to the root pipeline. This connection is defined by Gecko essentially, so it has all the info it needs to answer the question if a pipeline was actually built.

Can you confirm that this is just needed because it's more convenient to track this on our side?

staktrace · 2018-07-05T13:56:52Z

Agreed - this is mostly for convenience because we can have multiple scene build requests in flight at the same time and it to track it on the gecko side means we'd have to duplicate a bunch of stuff there. It seemed easier/more reliable to do it this way.

staktrace · 2018-07-05T13:59:19Z

(Also it's a little complicated on the Gecko side because the knowledge of whether the content display list was attached is buried deep in the display list creation, and extracting that and associating it with data from the content process is messy)

kvark · 2018-07-05T14:39:38Z

webrender/src/renderer.rs

@@ -1833,7 +1833,10 @@ impl Renderer {

    /// Returns the Epoch of the current frame in a pipeline.
    pub fn current_epoch(&self, pipeline_id: PipelineId) -> Option<Epoch> {
-        self.pipeline_info.epochs.get(&pipeline_id).cloned()
+        self.pipeline_info.epochs


is the current epoch useful for non-built pipelines?

Yes, knowing the epoch lets us know which gecko transaction has been completed, and lets us fire the MozAfterPaint event on the gecko side, which is the desired goal of this change.

but if the pipeline isn't built, it's not painted either, so how does MozAfterPaint make sense?

Because Gecko. You could consider it a bug, but in this case it's easier to bug-compatible with gecko than rewrite large chunks of the gecko frontend :(

Yeah it should really be called MozAfterPrepareToComposite or MozReadyToCompositeInstantly.

ok, @mstange , but it's not even prepared/ready if it's not built!

Maybe MozCompositorHasSomeIdeaThatThisExists :)

Actually, sorry, I was mixing up MozAfterPaint with MozLayerTreeReady. I concur with kats now; Gecko is being stupid and is using MozAfterPaint in a confused way, and matching the bug is an easy way out.

kvark · 2018-07-05T14:39:38Z

webrender/src/scene.rs

@@ -99,7 +99,7 @@ pub struct ScenePipeline {
 pub struct Scene {
    pub root_pipeline_id: Option<PipelineId>,
    pub pipelines: FastHashMap<PipelineId, Arc<ScenePipeline>>,
-    pub pipeline_epochs: FastHashMap<PipelineId, Epoch>,
+    pub pipeline_epochs: FastHashMap<PipelineId, (Epoch, BuildState)>,


hmm, this seems misplaced. Scene is the source for building, it's not supposed to know about build artifacts (like this flag)

Ok. I should be able to take this out, will update the patch.

@kvark Would the frame builder be a better place for this? The flattener can construct the map with all the built and unbuilt entries, pass it to the frame builder, and the render_backend can read it from the frame builder in updated_pipeline_info instead of the from the scene.

sounds reasonable

kvark · 2018-07-05T14:39:38Z

webrender_api/src/api.rs

+// attached to the root pipeline via iframe items.
+#[repr(C)]
+#[derive(Copy, Clone, Debug, Deserialize, Serialize)]
+pub struct BuildState(pub bool);


Any reason this is not just an enum?

Not really. I can change it to an enum.

staktrace · 2018-07-05T19:30:15Z

This is turning out more complicated than I thought. Right now there is code in update_epoch that sends a FrameMsg::UpdateEpoch. Ordinarily, after scene building, the new_scene only has the built pipelines in its pipeline_epochs map, but this FrameMsg::UpdateEpoch means that pipelines that were not built but were updated also end up in the new scene's pipeline_epochs map. And so right now when we send the PipelineInfo to gecko, it already contains a mix of built and not-built pipelines, and apparently there are tests that rely on this behaviour.

This patch as-is doesn't preserve this behaviour correctly, although it's simple enough to modify the patch to do so. The problem is preserving this behaviour without putting the BuildState on the scene's copy of the pipeline_epoch map (i.e. the change requested in the review comment above). We need a way to distinguish between pipelines added via scene::set_display_list and scene::update_epoch because the former should be marked as NotBuilt and the latter should be marked as Built, but if we don't store the BuildState on the scene then we lose this information.

staktrace · 2018-07-05T20:01:33Z

We need a way to distinguish between pipelines added via scene::set_display_list and scene::update_epoch because the former should be marked as NotBuilt and the latter should be marked as Built, but if we don't store the BuildState on the scene then we lose this information.

I realized we can do this by checking if scene.pipelines has an entry for the pipeline id. Doing try pushes and stuff to verify. Although honestly it doesn't feel like the best solution so I'll let it simmer a bit more. It might be possible to remove the Built vs NotBuilt distinction entirely.

kvark · 2018-07-05T20:02:13Z

Interesting! What if we store the pipeline epoch in the pipeline itself? This way, we can trivially find out if the pipeline was added with a display list by comparing it's epoch with the map key.

staktrace · 2018-07-06T10:40:07Z

So yeah, I don't think we need the distinction between "built" and "not built" pipelines on the gecko side. As long as all the pipeline epochs get propagated from the pending scene to the new scene things are good. Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31c488c1216646ac325e71f2cd527ea8debd79c6

The WR patch is much simpler, as it is mostly just code deletion now. I don't know if servo relies on the pipeline info only containing epochs for built scenes or not, that's the only potential concern. But really the code as it is now is not really providing a easily-describable set of epochs in the pipeline info because of this update_epoch business so if servo is relying on it they're probably going to have the same issues gecko does, and this patch should help.

kvark · 2018-07-06T16:51:36Z

Haven't seen such a clean try push in a while :)
I guess the intermittents are getting nailed (perhaps, even thanks to Pernosco?) faster than we introduce them.

Looking at the Servo code briefly, I'm not seeing them using the epochs.
@bors-servo r+

bors-servo · 2018-07-06T16:51:37Z

📌 Commit 9e62b43 has been approved by kvark

bors-servo · 2018-07-06T16:51:40Z

⌛ Testing commit 9e62b43 with merge d4f5b4f...


        Auto merge of #2865 - staktrace:buildstate, r=kvark

Store the built state in the PipelineInfo This adds a flag to the entries in the PipelineInfo that records whether or not a pipeline was actually built at a given epoch. It is possible for pipelines to have display lists that are not built if they are not connected to the root pipeline via a chain of iframe items, and in some cases Gecko needs to be able to distinguish between whether a pipeline was built or not during a particular render.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2865)

bors-servo · 2018-07-06T17:01:38Z

💔 Test failed - status-taskcluster


        Copy all the pipeline epoch information to the new scene.

Without this patch, the pipeline_epoch info on the new scene contains just the pipelines that were "built" as part of the scene build, plus pipelines on for which update_epoch was called. This is kind of random, and there doesn't appear to be any real reason to omit some of the pipelines. For gecko it is desirable to the epoch information for all pipelines, even the ones that weren't built, and for consistency we can just always propagate the epoch info over to the new scene during the scene build.

staktrace · 2018-07-06T17:17:41Z

Trivial merge error; I rebased the patch and took out the now-unneeded "use Epoch". ^

kvark · 2018-07-06T17:57:51Z

@bors-servo r+

bors-servo · 2018-07-06T17:57:51Z

📌 Commit b8bb282 has been approved by kvark

bors-servo · 2018-07-06T17:57:54Z

⌛ Testing commit b8bb282 with merge 0e95636...


        Auto merge of #2865 - staktrace:buildstate, r=kvark

Store the built state in the PipelineInfo This adds a flag to the entries in the PipelineInfo that records whether or not a pipeline was actually built at a given epoch. It is possible for pipelines to have display lists that are not built if they are not connected to the root pipeline via a chain of iframe items, and in some cases Gecko needs to be able to distinguish between whether a pipeline was built or not during a particular render.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2865)

bors-servo · 2018-07-06T20:04:09Z

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

kvark reviewed Jul 5, 2018

View changes

staktrace force-pushed the staktrace:buildstate branch from 79bddeb to 9e62b43 Jul 6, 2018

staktrace force-pushed the staktrace:buildstate branch from 9e62b43 to b8bb282 Jul 6, 2018

bors-servo merged commit b8bb282 into servo:master Jul 6, 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

staktrace deleted the staktrace:buildstate branch Jul 6, 2018

bors-servo mentioned this pull request Jul 6, 2018

Inherit the clip chain of IFrame #2875

Merged

servo / webrender

Store the built state in the PipelineInfo #2865

Store the built state in the PipelineInfo #2865

staktrace commented Jul 4, 2018 •

edited by larsbergstrom

staktrace commented Jul 4, 2018

kvark commented Jul 5, 2018

staktrace commented Jul 5, 2018

staktrace commented Jul 5, 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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

staktrace commented Jul 5, 2018

staktrace commented Jul 5, 2018

kvark commented Jul 5, 2018

staktrace commented Jul 6, 2018

kvark commented Jul 6, 2018

bors-servo commented Jul 6, 2018

bors-servo commented Jul 6, 2018

bors-servo commented Jul 6, 2018

staktrace commented Jul 6, 2018

kvark commented Jul 6, 2018

bors-servo commented Jul 6, 2018

bors-servo commented Jul 6, 2018

bors-servo commented Jul 6, 2018

servo / webrender

Join GitHub today

Store the built state in the PipelineInfo #2865

Store the built state in the PipelineInfo #2865

Conversation

staktrace commented Jul 4, 2018 • edited by larsbergstrom

staktrace commented Jul 4, 2018

kvark commented Jul 5, 2018

staktrace commented Jul 5, 2018

staktrace commented Jul 5, 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.

mstange Jul 5, 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.

staktrace commented Jul 5, 2018

staktrace commented Jul 5, 2018

kvark commented Jul 5, 2018

staktrace commented Jul 6, 2018

kvark commented Jul 6, 2018

bors-servo commented Jul 6, 2018

bors-servo commented Jul 6, 2018

bors-servo commented Jul 6, 2018

staktrace commented Jul 6, 2018

kvark commented Jul 6, 2018

bors-servo commented Jul 6, 2018

bors-servo commented Jul 6, 2018

bors-servo commented Jul 6, 2018

staktrace commented Jul 4, 2018 •

edited by larsbergstrom

mstange Jul 5, 2018 •

edited