Closed Bug 1350638 Opened 7 years ago Closed 7 years ago

Eliminate the PCompositorBridge::Msg_GetCompositorOptions sync IPC

Categories

(Core :: Graphics: Layers, enhancement)

Product:
Core
Component:
Graphics: Layers
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: kats)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files)

Bug 1350638 - Remove sync GetCompositorOptions call in TabChild::InitRenderingState.
59 bytes, text/x-review-board-request
dvander
: review+
Details
Bug 1350638 - Remove sync GetCompositorOptions call added in the last patch.
59 bytes, text/x-review-board-request
dvander
: review+
Details
Bug 1350638 - Remove sync GetCompositorOptions call in TabChild::ReinitRenderingState.
59 bytes, text/x-review-board-request
dvander
: review+
Details
Bug 1350638 - Remove the GetCompositorOptions IPC message as it is no longer used.
59 bytes, text/x-review-board-request
dvander
: review+
Details 
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
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 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 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 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+
^ Fixed a typo in the first patch's commit message and addressed the nit for part 2.
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
Performance Impact: --- → P1
Whiteboard: [gfx-noted][qf:p1] → [gfx-noted]
You need to log in before you can comment on or make changes to this bug.