1128489 - Use direct call between SettingsService and SettingsRequestManager

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[:gerard-majax]

+++ This bug was initially created as a clone of Bug #1124622 +++

One of the reason of the latency at boot time documented in bug 1124622 comes from the way communication is done between SettingsService and SettingsRequestManager: via async messaging.

This makes messages taking quite some time to be delivered. Figures shows that we can win 3-4 secs on boot time (from gecko startup to calling shell.start()) by changing the communication and make it direct between both objects rather than making use of SendAsyncMessage/ReceiveMessage.

Kyle, I know this is breaking your heart and that you wanted to have a consistent way of communicating for SettingsManager and SettingsService, so do you agree on the idea of changing this communication ?

We should be allowed to do this because those two components do live in the same process.

Comment 1

[:gerard-majax]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d519f3bca0f4

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Oh, yeah, go for it. Consistency was nice but speed is important, and gwagner and I have discussed doing far worse things now too. :)

Comment 3

[:gerard-majax]

Attachment: Direct communication between SettingsService and SettingsRequestManager r=qdot

Comment 4

[:gerard-majax]

Attachment: Direct communication between SettingsService and SettingsRequestManager r=qdot

Comment 5

[:gerard-majax]

Updated the patch: I've noticed that unregisterLock() was done before we received the Settings:Finalize:OK message, so I've moved it a little bit.

Matching try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dacfd3573f11

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

lgtm

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85c0e2ca852d

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85c0e2ca852d