Improve WebRenderBridgeChild::SyncWithCompositor()

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: sotaro, Assigned: kats)

Tracking

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox62 fixed)

Details

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

Attachments

(3 attachments)

Reporter

Description

2 years ago
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.
Reporter

Updated

2 years ago
Blocks: 1411472
Reporter

Updated

2 years ago
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.
Depends on: 1464086
Comment hidden (mozreview-request)
@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 6

a year ago
mozreview-review
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+
Reporter

Comment 7

a year ago
mozreview-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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Reporter

Comment 14

a year ago
mozreview-review
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+
Reporter

Comment 15

a year ago
mozreview-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.
Reporter

Comment 16

a year ago
Except Comment 15, attachment 8980878 [details] looks good:)
Assignee

Comment 17

a year ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Reporter

Comment 22

a year ago
mozreview-review
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+

Comment 23

a year ago
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.