ApplyAsyncImages() is called in WebRenderBridgeParent::RecvSetDisplayList(). It seems redundant. We could set default display list in WebRenderBridgeParent::AddPipelineIdForCompositable().

Comment on attachment 8991563 [details] [diff] [review] patch - Remove AsyncImagePipelineManager::ApplyAsyncImages() call from WebRenderBridgeParent::RecvSetDisplayList() Review of attachment 8991563 [details] [diff] [review]: ----------------------------------------------------------------- Seems reasonable ::: gfx/layers/wr/WebRenderBridgeParent.cpp @@ +1076,5 @@ > mAsyncCompositables.emplace(wr::AsUint64(aPipelineId), wrHost); > mAsyncImageManager->AddAsyncImagePipeline(aPipelineId, wrHost); > > + // The display list that we're sending to WR in the transaction might contain > + // references to the pipelines that we just added to the async image manager. Replace the first sentence of this comment with this: "If this is being called from WebRenderBridgeParent::RecvSetDisplayList, then aTxn might contain a display list that references pipelines that we just added to the async image manager." I think that makes it a bit more clear which display list is being referred to.

(In reply to Sotaro Ikeda [:sotaro] from comment #2) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=d5442f352c8c203fb74f0a43d1439f7ef4a74c6d&selectedJob=1 > 87795752 Some reftests failure on windows. attachment 8991768 [details] [diff] [review] defters actual display list update to ApplyAsyncImages() in WebRenderBridgeParent::CompositeToTarget(). But the display list update happened async way. Then snapshot did not include valid display lists of async image pipelines.

(In reply to Sotaro Ikeda [:sotaro] from comment #5) > [review] defters actual display list update to ApplyAsyncImages() in > WebRenderBridgeParent::CompositeToTarget(). But the display list update > happened async way. Then snapshot did not include valid display lists of > async image pipelines. By "actual display list update" I assume you mean the non-empty display list? If so then maybe a better solution is to do that display list update when we get the TOpUpdateAsyncImagePipeline command in WebRenderBridgeParent. We would effectively be changing it so that instead of sending the async image pipelines along with the main pipeline, we'd send the async image pipeline whenever we get it from the content. In the case of getting a snapshot, the existing FlushSceneBuilds call would flush both the main pipeline and the async image pipelines before going to CompositeToTarget. Also, it's not really clear to me why we need to do this at all in order to fix bug 1467768. I thought we would be able to fix that without having to move this stuff around.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6) > > Also, it's not really clear to me why we need to do this at all in order to > fix bug 1467768. I thought we would be able to fix that without having to > move this stuff around. Thanks for the advice. I agree. I am going to remove the dependency. This bug is for removing ApplyAsyncImages() call in RecvSetDisplayList() for reducing unnecessary task. At first, I thought that ApplyAsyncImages() call could be only from CompositeToTarget(). But it was bad.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6) > By "actual display list update" I assume you mean the non-empty display > list? If so then maybe a better solution is to do that display list update > when we get the TOpUpdateAsyncImagePipeline command in > WebRenderBridgeParent. UpdateAsyncImagePipeline is not enough to handle all cases, since it is not called during WebRenderBridgeParent::RecvEmptyTransaction(). And updating image key is necessary when wr::ImageKey is created for AsyncImagePipeline.

(In reply to Sotaro Ikeda [:sotaro] from comment #8) > > UpdateAsyncImagePipeline is not enough to handle all cases, since it is not > called during WebRenderBridgeParent::RecvEmptyTransaction(). And updating > image key is necessary when wr::ImageKey is created for AsyncImagePipeline. Ah, to address this bug, UpdateAsyncImagePipeline seems enough. Another case is still handled by ApplyAsyncImages() in CompositeToTarget().

attachment 8992826 [details] [diff] [review] uses OpUpdateAsyncImagePipeline to apply AsyncImage for Pipeline and adds OpUpdatedAsyncImagePipeline to apply AsyncImage for Pipeline during EmptyTransaction. With then, all applying AsyncImage for canvas is done with them. In WebRenderBridgeParent::CompositeToTarget(), applying AsyncImages are done only for videos(via ImageBridge).

Comment on attachment 8992826 [details] [diff] [review] patch - Remove AsyncImagePipelineManager::ApplyAsyncImages() call from WebRenderBridgeParent::RecvSetDisplayList() :kats, can you review the patch again?

Comment on attachment 8992826 [details] [diff] [review] patch - Remove AsyncImagePipelineManager::ApplyAsyncImages() call from WebRenderBridgeParent::RecvSetDisplayList() Review of attachment 8992826 [details] [diff] [review]: ----------------------------------------------------------------- This seems reasonable. It would have been easier to review if you extracted that helper function in one patch and did the rest of the work in a second patch.

Thanks!

Pushed by sikeda@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d99ef0aa6617 Remove AsyncImagePipelineManager::ApplyAsyncImages() call from WebRenderBridgeParent::RecvSetDisplayList() r=kats

Perf win! == Change summary for alert #14437 (as of Wed, 18 Jul 2018 21:14:53 GMT) == Improvements: 27% glterrain windows10-64-qr opt e10s stylo 2.28 -> 1.66 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14437