De-syncify layer id mapping in the compositor

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics: Layers
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: dvander, Assigned: dvander, NeedInfo)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 months ago
There's two pieces of low-hanging fruit for sync messages in the UI process. The first is the initial layer id mapping, which can be async as long as EnsureGPUReady() will ultimately block on it being received.

The second is that the allocation message is sent immediately before sending NotifyChildCreated, which is also sync. There's no need for that and we can create a combined sync message.
(Assignee)

Comment 1

3 months ago
Created attachment 8851382 [details] [diff] [review]
part 1, combine messages

When using the GPU process, send a single message that performs both the layer tree mapping and the NotifyChildCreated logic.
Attachment #8851382 - Flags: review?(rhunt)
(Assignee)

Comment 2

3 months ago
Created attachment 8851386 [details] [diff] [review]
part 2, fold into init message

As for the initial mapping on startup, I think we can just fold this into the Init message, which is already async.
Attachment #8851386 - Flags: review?(rhunt)

Updated

3 months ago
Attachment #8851382 - Flags: review?(rhunt) → review+

Updated

3 months ago
Attachment #8851386 - Flags: review?(rhunt) → review+

Comment 3

3 months ago
Comment on attachment 8851382 [details] [diff] [review]
part 1, combine messages

Review of attachment 8851382 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/RenderFrameParent.cpp
@@ +128,3 @@
>    } else if (XRE_IsContentProcess()) {
>      ContentChild::GetSingleton()->SendAllocateLayerTreeId(browser->Manager()->ChildID(), browser->GetTabId(), &mLayersId);
>      CompositorBridgeChild::Get()->SendNotifyChildCreated(mLayersId);

I'm not sure what the state of nested browser processes is, but if this code is converted we should be able to get rid of the NotifyChild message. Not sure if it's worth the work.
(Assignee)

Comment 4

3 months ago
Comment on attachment 8851382 [details] [diff] [review]
part 1, combine messages

Bill, IPC peer r? for new sync message
Attachment #8851382 - Flags: review?(wmccloskey)
Comment on attachment 8851382 [details] [diff] [review]
part 1, combine messages

Review of attachment 8851382 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/PCompositorBridge.ipdl
@@ +175,5 @@
>    // See bug 1316632 comment #33 for why this has to be sync. Otherwise,
>    // there are ordering issues with SendPLayerTransactionConstructor.
>    sync NotifyChildCreated(uint64_t id);
>  
> +  // This version of NotifyChildCreated also performs a layer tree mapping.

Can you copy the comment about "See bug 1316632 comment #33" to this message as well. I don't want to lose it if we drop NotifyChildCreated.
Attachment #8851382 - Flags: review?(wmccloskey) → review+

Comment 6

3 months ago
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f033452487fc
When using the GPU process, combine layer ownership and window mapping into a single IPC message. (bug 1350660 part 1, r=rhunt, r=billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/13d070200dfe
Don't synchronously send the initial layer tree mapping list to the GPU process. (bug 1350660 part 2, r=rhunt)

Comment 7

3 months ago
I had to back these out because they were conflicting with bug 1351777 while I was trying to do merges this evening. Feel free to rebase and reland.
Flags: needinfo?(dvander)

Comment 8

3 months ago
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb665191fd2
Backed out 2 changesets for causing merge conflicts a=backout

Comment 9

3 months ago
Backout by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/7b2da53804c6
Backed out 2 changesets for causing merge conflicts a=backout

Comment 10

3 months ago
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/599e7c393321
When using the GPU process, combine layer ownership and window mapping into a single IPC message. (bug 1350660 part 1, r=rhunt, r=billm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8605e9e4f7bb
Don't synchronously send the initial layer tree mapping list to the GPU process. (bug 1350660 part 2, r=rhunt)

Comment 11

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/599e7c393321
https://hg.mozilla.org/mozilla-central/rev/8605e9e4f7bb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.