Closed Bug 1342636 Opened 3 years ago Closed 3 years ago

Remove PHal::Msg_GetCurrentScreenConfiguration sync IPC

Categories

(Core :: Hardware Abstraction Layer (HAL), defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

This sync IPC is pretty bad: <https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-02-22&keys=PHal%253A%253AMsg_GetCurrentScreenConfiguration!PCookieService%253A%253AMsg_GetCookieString!PContent%253A%253AMsg_RpcMessage!PAPZCTreeManager%253A%253AMsg_ReceiveMouseInputEvent&max_channel_version=nightly%252F54&measure=IPC_SYNC_LATENCY_MS&min_channel_version=null&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-02-02&table=0&trim=1&use_submission_date=0>

Start	End	IPC_SYNC_LATENCY_MS Count
0	1	0 (0%)
1	3	430.42k (50.74%)
3	8	164.9k (19.44%)
8	20	107.05k (12.62%)
20	50	74.12k (8.74%)
50	126	41.41k (4.88%)
126	317	18.3k (2.16%)
317	796	7.43k (0.88%)
796	2k	3.26k (0.38%)
2k	Infinity	1.33k (0.16%)

And it's entirely unnecessary, since we already have the necessary API to listen on the parent side and notify the child about any changes in screen information. <http://searchfox.org/mozilla-central/rev/c9ccb8ec1f5acda78e6344ffb87aa3a409031e3d/hal/Hal.h#374>  It's pretty easy to switch this around so that the parent notifies the child about screen changes and the child just uses its cache of the information.
There seems to be a pattern where the child mirrors a value in the parent. We already do this across threads in https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/StateMirroring.h

Kanru - do we have a good utility class to do this?
Flags: needinfo?(kchen)
I think for all places where we use hal::GetCurrentScreenConfiguration could be replaced by nsIScreenManager after I fix bug 1194751.

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #1)
> There seems to be a pattern where the child mirrors a value in the parent.
> We already do this across threads in
> https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/StateMirroring.h
> 
> Kanru - do we have a good utility class to do this?

Not that I'm aware of. If we see this pattern often enough maybe we could create something similar in in IPC. Some code under hal/ use a observer model that intended to address this. 
Note sometimes the value in the parent is also computed on the fly. In that case we still have to change the parent to cache the values.
Flags: needinfo?(kchen)
I still haven't seen enough (In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #2)
> I think for all places where we use hal::GetCurrentScreenConfiguration could
> be replaced by nsIScreenManager after I fix bug 1194751.

Well but the nsIScreenManager API is really verbose to use for really basic use cases, whereas hal::GetCurrentScreenConfiguration() gives you a nice way to do simple things.  Turns out we already have code in the fallback HAL <http://searchfox.org/mozilla-central/rev/78ac0ceba97bd2deed847a8d0ae86ccf7a8887bf/hal/fallback/FallbackScreenConfiguration.cpp#24> that is implemented in terms of nsIScreenManager API, so we can just share it here, which would be a super clean and simple fix for this!

> (In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #1)
> > There seems to be a pattern where the child mirrors a value in the parent.
> > We already do this across threads in
> > https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/StateMirroring.h
> > 
> > Kanru - do we have a good utility class to do this?
> 
> Not that I'm aware of. If we see this pattern often enough maybe we could
> create something similar in in IPC. Some code under hal/ use a observer
> model that intended to address this. 
> Note sometimes the value in the parent is also computed on the fly. In that
> case we still have to change the parent to cache the values.

I think usually we either have some kind of an observer pattern that we can use to do this easily and IPDL makes it flexible to send various types of data, I don't think I have felt the need for a more general solution here.
This uses the underlying XPCOM API which works based on asyncronous
IPC.
Attachment #8845215 - Flags: review?(kchen)
Attachment #8845216 - Flags: review?(wmccloskey) → review+
Attachment #8845214 - Flags: review?(kchen) → review+
Attachment #8845215 - Flags: review?(kchen) → review+
I guess I can land this even before bug 1331680 lands, and that bug will remove the whole class of sync IPC all in one go!
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/589ee8b3364a
Part 1: Factor out the fallback screen configuration code into a shared header; r=kanru
https://hg.mozilla.org/integration/mozilla-inbound/rev/33ad33ac710c
Part 2: Switch to using the fallback implementation of GetCurrentScreenConfiguration in the content process; r=kanru
https://hg.mozilla.org/integration/mozilla-inbound/rev/966b6ec41dfc
Part 3: Remove the GetCurrentScreenConfiguration sync IPC message; r=billm
You need to log in before you can comment on or make changes to this bug.