Closed Bug 1417784 Opened 4 years ago Closed 3 years ago

Improve WebRenderBridgeChild::SyncWithCompositor()

Categories

(Core :: Graphics: WebRender, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox57 --- unaffected
firefox62 --- fixed

People

(Reporter: sotaro, Assigned: kats)

References

Details

(Whiteboard: [wr-reserve] [gfx-noted])

Attachments

(3 files)

The SyncWithCompositor() sends sync message to parent side. It expects that some buffers are read unlocked. But it does not work well with WebRender, since WebRender runs as multi-threaded way.

It would be nice if WebRenderBridgeParent could handle inflight DidComposite events. It could trigger read unlocking.
Blocks: 1411472
Whiteboard: [wr-mvp] [triage]
Priority: -- → P3
Whiteboard: [wr-mvp] [triage] → [wr-mvp] [triage][wr-reserve-candidate]
Whiteboard: [wr-mvp] [triage][wr-reserve-candidate] → [wr-reserve]
Whiteboard: [wr-reserve] → [wr-reserve] [gfx-noted]
I believe fixing this would fix crashes due to buffer over-production that I'm seeing in CI with async scene building enabled. See https://treeherder.mozilla.org/logviewer.html#?job_id=178422316&repo=try&lineNumber=18286 for a sample log.

I'll see if I can write a patch for this.
Assignee: nobody → bugmail
Blocks: 1452845
Hm, the ASB-enabled try push still has a debug gpu job with the same assertion. Will investigate more.
@froydnj: need an IPC peer to r+ the sync IPC message. Note that it's not actually adding a new IPC message, it's effectively just moving a pre-existing IPC message from PCompositorBridge to PWebRenderBridge for the webrender-enabled scenario.

Try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=0da49e5875cfb2540cc9c8998bfa1229ed680aa2 includes this patch. I had a previous try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=abe896785893f58feab65ddb65d2701c6052144b which had mostly the same code and solved the buffer over-production so I'm fairly confident this is going to be fine.
No longer depends on: 1464086
Comment on attachment 8980408 [details]
Bug 1417784 - Properly implement SyncWithCompositor for WebRender.

https://reviewboard.mozilla.org/r/246576/#review252716

r=me on the sync ipc message.
Attachment #8980408 - Flags: review?(nfroyd) → review+
Comment on attachment 8980408 [details]
Bug 1417784 - Properly implement SyncWithCompositor for WebRender.

https://reviewboard.mozilla.org/r/246576/#review252770
Attachment #8980408 - Flags: review?(sotaro.ikeda.g) → review+
These patches are insufficient to make the SyncWithCompositor work. I did a push with more logging and got the result at [1]. The problem is that after the renderer thread does the rendering, it sends a NotifyPipelineRendered call back to the compositor thread, and that's the thing that actually frees the textures. And that can't run because the compositor thread is blocked inside SyncWithCompositor. It's basically the same problem that I fixed by adding the ScheduleGenerateFrame() call, but in a different place. This one is less easy to fix though. I'll think about it.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=180321774&repo=try&lineNumber=6911
Ok, I figured out a solution, and it seems to work. Try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b7fc2e95e9becec455279591bd28c8eccf5f525 - the important thing here is that none of the failures are due to the buffer over-production error that were occurring before. There are some persistent or high-frequency failures, but I'll deal with those separately in another dependency of bug 1452845.

Updated patches coming.
Note that I made two changes in part 3, which you can see in the interdiff at https://reviewboard.mozilla.org/r/246576/diff/1-2/
- Ensure that FlushFrameGeneration is always called on the root WRBP because otherwise setting mForceRendering has no effect. This was a silly mistake in my original patch
- Add a call to process the pipeline updates in the async image manager from SyncWithCompositor. This is so that we actually free the old textures before returning, and this is what i need the first two patches for.
Comment on attachment 8980877 [details]
Bug 1417784 - Use a CompositorBridgeParent* instead of a base class pointer.

https://reviewboard.mozilla.org/r/247064/#review253164
Attachment #8980877 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8980878 [details]
Bug 1417784 - Shift how the AsyncImagePipelineManager is notified of updates.

https://reviewboard.mozilla.org/r/247066/#review253192

::: gfx/webrender_bindings/RenderThread.cpp:287
(Diff revision 1)
>    TimeStamp end = TimeStamp::Now();
>  
>    auto info = renderer->FlushPipelineInfo();
> +  RefPtr<layers::AsyncImagePipelineManager> pipelineMgr =
> +      renderer->GetCompositorBridge()->GetAsyncImagePipelineManager();
> +  if (pipelineMgr) {

Does it expect that CompositorBridgeParent::mAsyncImageManager becomes nulptr during its usage? If it is, access of mAsyncImageManager is not protected by mutex, then there could be a risk of using stale pointer like Bug 1441498 Comment 8.

Though the situation seems not happen with current CompositorBridgeParent and AsyncImagePipelineManager source. Since root WebRenderAPI seems to be destroyed before CompositorBridgeParent::mAsyncImageManager becomes nulptr.
Except Comment 15, attachment 8980878 [details] looks good:)
Comment on attachment 8980878 [details]
Bug 1417784 - Shift how the AsyncImagePipelineManager is notified of updates.

https://reviewboard.mozilla.org/r/247066/#review253316

::: gfx/webrender_bindings/RenderThread.cpp:287
(Diff revision 1)
>    TimeStamp end = TimeStamp::Now();
>  
>    auto info = renderer->FlushPipelineInfo();
> +  RefPtr<layers::AsyncImagePipelineManager> pipelineMgr =
> +      renderer->GetCompositorBridge()->GetAsyncImagePipelineManager();
> +  if (pipelineMgr) {

I don't think there is a race here. The CompositorBridgeParent::mAsyncImageManager is set to null at [1], which (as the comment there says) happens after the WebRenderAPI is destroyed (as part of the mWrBridge->Destroy() call a few lines up). And the WebRenderAPI destructor blocks until the renderer thread is done at [2]. So basically this code should never run at a point where CompositorBridge::mAsyncImageManager for that pipeline is getting modified. In fact I think pipelineMgr will always be non-null here, so maybe I should replace the if condition with an assert? I can also add a comment.

[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/gfx/layers/ipc/CompositorBridgeParent.cpp#518
[2] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/gfx/webrender_bindings/WebRenderAPI.cpp#341
Comment on attachment 8980878 [details]
Bug 1417784 - Shift how the AsyncImagePipelineManager is notified of updates.

https://reviewboard.mozilla.org/r/247066/#review253388

Looks good!
Attachment #8980878 - Flags: review?(sotaro.ikeda.g) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6fd83af52c3
Use a CompositorBridgeParent* instead of a base class pointer. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/6a1252b68f4f
Shift how the AsyncImagePipelineManager is notified of updates. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/d6ea05eb01ac
Properly implement SyncWithCompositor for WebRender. r=froydnj,sotaro
You need to log in before you can comment on or make changes to this bug.