Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Eliminate the PCompositorBridge::Msg_GetCompositorOptions sync IPC

RESOLVED FIXED in Firefox 55

Status

()

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

People

(Reporter: Ehsan, Assigned: kats)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [gfx-noted][qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

Median time according to telemetry: 7ms.
Flags: needinfo?(bas)
Kats, do we still need this?
Flags: needinfo?(bas) → needinfo?(bugmail)
Whiteboard: [gfx-noted]
We should be able to get rid of it.
Assignee: nobody → bugmail
Flags: needinfo?(bugmail)
Hm, actually getting rid of it might not be so easy. When I added it, it actually replaced another sync IPC message [1]. So even if I roll back the CompositorOptions changes we'll still end up with that other sync IPC message. I'll need to give this some thought.

[1] https://hg.mozilla.org/mozilla-central/rev/cfebe97561fb#l10.31
I looked around at the code and I think we can piggy back this into the RenderFrameParent via the SendNotifyChildCreated message, and then send it down to the TabChild over SendInitRendering, or something along those lines. I'll wait for bug 1350660 is on central before trying this.
Whiteboard: [gfx-noted] → [gfx-noted][qf]
Whiteboard: [gfx-noted][qf] → [gfx-noted][qf:p1]
First I tried just getting rid of the GetCompositorOptions entirely. The only reason we need it in the content process is to know if APZ is enabled or not in a particular TabChild, and I was hoping we can just make that determination locally in the TabChild. I did a try push with that [1] and it started showing a bunch of failures. I don't really understand why but I put that approach on hold and went to implement what I described in comment 4.

I was able to piggyback CompositorOptions onto other sync IPC messages like NotifyChildCreated. However there are two complications. One is that TabChild::ReinitRendering also calls GetCompositorOptions, and I don't see any easy way to avoid that. It should be possible but I'll need to look a little harder.

The other complication is that in getting rid of the GetCompositorOptions from TabChild::InitRendering, I had to add one after a call to SendAdoptChild in RenderFrameParent. If AdoptChild had already been sync I could have just reused that but it's async and I didn't want to make it sync, so I had to add a call to GetCompositorOptions there. There might be a way to get rid of it there too, I will keep poking around.

Even with these two caveats I think what I have will reduce the absolute number of sync IPC calls of GetCompositorOptions, because it removes it from one highly-used place and adds it to a less-used place, so it should be a net win.

For now I did a try push with the patch I have so far [2], to see if that breaks anything.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa5fbc7fd16aedab21a0bc1dd78935328e563f99
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c14fb43e61d450904256f8e3afa45e8056a3e2d
I think I figured out the fixes for the two complications described above. New try push (Linux and Android only for now, if it's green I'll do another with everything):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3ec87f65229a0e3402fc7d9f7e4cf53341deac9
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b533659e11d0790a11f74f7f21cd1be0220fe762
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab96dfa150ab797b53999606519b66e961501f25

Looks pretty good.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

3 months ago
mozreview-review
Comment on attachment 8856284 [details]
Bug 1350638 - Remove sync GetCompositorOptions call in TabChild::InitRenderingState.

https://reviewboard.mozilla.org/r/128186/#review130664

This is great. I wonder if something similar could be used to get rid of the synchronous PLayerTransaction allocation - we should know the TextureFactoryIdentifier as soon as we associate the layers id.
Attachment #8856284 - Flags: review?(dvander) → review+

Comment 13

3 months ago
mozreview-review
Comment on attachment 8856285 [details]
Bug 1350638 - Remove sync GetCompositorOptions call added in the last patch.

https://reviewboard.mozilla.org/r/128188/#review130666

::: gfx/ipc/CompositorOptions.h:50
(Diff revision 1)
>    bool UseAPZ() const { return mUseAPZ; }
>    bool UseWebRender() const { return mUseWebRender; }
>  
> +  bool operator==(const CompositorOptions& aOther) {
> +    return mUseAPZ == aOther.mUseAPZ
> +        && mUseWebRender == aOther.mUseWebRender;

nit: && should be on the previous line (the style guide has an exception to the line-break rule for the && and || operators)
Attachment #8856285 - Flags: review?(dvander) → review+

Comment 14

3 months ago
mozreview-review
Comment on attachment 8856286 [details]
Bug 1350638 - Remove sync GetCompositorOptions call in TabChild::ReinitRenderingState.

https://reviewboard.mozilla.org/r/128190/#review130668
Attachment #8856286 - Flags: review?(dvander) → review+

Comment 15

3 months ago
mozreview-review
Comment on attachment 8856287 [details]
Bug 1350638 - Remove the GetCompositorOptions IPC message as it is no longer used.

https://reviewboard.mozilla.org/r/128192/#review130676
Attachment #8856287 - Flags: review?(dvander) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
^ Fixed a typo in the first patch's commit message and addressed the nit for part 2.

Comment 21

3 months ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3ddc4081b94
Remove sync GetCompositorOptions call in TabChild::InitRenderingState. r=dvander
https://hg.mozilla.org/integration/autoland/rev/dbfa42b56f1b
Remove sync GetCompositorOptions call added in the last patch. r=dvander
https://hg.mozilla.org/integration/autoland/rev/4b455ab566a2
Remove sync GetCompositorOptions call in TabChild::ReinitRenderingState. r=dvander
https://hg.mozilla.org/integration/autoland/rev/1a8d35bbcb2f
Remove the GetCompositorOptions IPC message as it is no longer used. r=dvander
https://hg.mozilla.org/mozilla-central/rev/e3ddc4081b94
https://hg.mozilla.org/mozilla-central/rev/dbfa42b56f1b
https://hg.mozilla.org/mozilla-central/rev/4b455ab566a2
https://hg.mozilla.org/mozilla-central/rev/1a8d35bbcb2f
Status: NEW → 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.