Closed Bug 1126119 Opened 5 years ago Closed 5 years ago

sync SystemMessageManager:HasPendingMessages takes too much time (200+ms)

Categories

(Core :: DOM: Device Interfaces, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: kanru, Assigned: kanru)

References

Details

Attachments

(2 files, 2 obsolete files)

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

From bug 1102715 we found that the sync IPC messageSystemMessageManager:HasPendingMessages takes too much time and blocks app startup. Similar to bug 1112989 maybe we could send this information along with LoadURL to child.
I think HasPendingMessage only guarantees that when it returns true, message handlers will be called as soon as possible asynchronously. When it returns false, message handlers may still be called since messages could be dispatched at any time. I.e. it could be false negative.

We could send first HasPendingMessage state with TabParent::LoadURL to child and asynchronously update live TabParents their HasPendingMessage state. So app could get accurate info at startup and almost accurate info if they want to check that later (I think this use case is rare though).
Flags: needinfo?(fabrice)
(In reply to Kan-Ru Chen [:kanru] from comment #1)
> I think HasPendingMessage only guarantees that when it returns true, message
> handlers will be called as soon as possible asynchronously. When it returns
> false, message handlers may still be called since messages could be
> dispatched at any time. I.e. it could be false negative.

HasPendingMessage() is designed to be used before SetMessageHandler(). It just tells that once you call SetMessageHandler() there will be messages ready to be delivered.

> We could send first HasPendingMessage state with TabParent::LoadURL to child
> and asynchronously update live TabParents their HasPendingMessage state. So
> app could get accurate info at startup and almost accurate info if they want
> to check that later (I think this use case is rare though).

Yeah, the "almost accurate" part is annoying in theory, but I agree that our code patterns will not hit that.
Flags: needinfo?(fabrice)
This bug is referenced in bug 1086963, which is about startup time for the Gallery app.  Since you've pointed out that this is a bottleneck, I'm likely to modify Gallery to remove the call to hasPendingMessage(), and instead just check location.href.

This may still be a good optimization to do, but in terms of speeding up Gallery, it probably will not help if I remove the hasPendingMessage call in that app.
Attached patch patch (obsolete) — Splinter Review
After patch the hasPendingMessage check disappeared completely.

https://people.mozilla.org/~bgirard/cleopatra/#report=05e2a0f064e9cdc5d3c82c349c32a23835d46fcc&filter=[{"type"%3A"RangeSampleFilter","start"%3A1477,"end"%3A2129}]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38f4d9173091
Attachment #8558950 - Flags: review?(fabrice)
Comment on attachment 8558950 [details] [diff] [review]
patch

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

I should also remove the old hasPendingMessage message handler.

::: dom/messages/SystemMessageManager.js
@@ +195,5 @@
>  
> +    let cache = Cc["@mozilla.org/system-message-cache;1"]
> +                  .getService(Ci.nsISystemMessageCache);
> +
> +    return cache.hasPendingMessage(aType, this._pageURL, this._manifestURL);

I should put a comment here:
/*
 * NB: If the system message is fired after we received the cache and before we
 *     registered the pageURL we will get false negative however this is unlikely
 *     and will do no harm.
 */
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → kchen
Attachment #8558950 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8558950 - Flags: review?(fabrice)
Attachment #8558983 - Flags: review?(fabrice)
Comment on attachment 8558983 [details] [diff] [review]
patch

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

the overall design looks fine, but I think there are some issues.
Also, a bunch of .orig files ended up in your patch.

::: dom/messages/SystemMessageCache.js
@@ +51,5 @@
> +
> +  receiveMessage: function(aMessage) {
> +    switch (aMessage.name) {
> +    case "SystemMessageCache:RefreshCache":
> +      this._pagesCache = aMessage.json;

nit: s/aMessage.json/aMessage.data

@@ +52,5 @@
> +  receiveMessage: function(aMessage) {
> +    switch (aMessage.name) {
> +    case "SystemMessageCache:RefreshCache":
> +      this._pagesCache = aMessage.json;
> +      debug("received RefreshCache " + JSON.stringify(aMessage.json));

please don't use stringify in unconditional debug()

::: dom/messages/SystemMessageInternal.js
@@ +182,5 @@
> +    this._pages.forEach(function(aPage) {
> +      if (aPage.manifestURL === aManifestURL) {
> +        pages.push(aPage);
> +      }
> +    }, this);

you don't need the 'this' here.

@@ +365,5 @@
> +    let manifestURL = aManifestURI.spec;
> +    let pages = this._findPagesForApp(manifestURL);
> +    let cache = [];
> +    pages.forEach(function(aPage) {
> +      if (aPage.pendingMessages.length != 0) {

why not move this condition in _findPagesForApp() ?

@@ +480,5 @@
>              winCounts[pageURL]++;
>            }
>          }
>  
> +        this.refreshCache(aMessage.target, msg.manifestURL);

msg.manifestURL is a string here, but refreshCache expected a nsIURI to access the .spec
Something's wrong!
Attachment #8558983 - Flags: review?(fabrice) → feedback+
Attachment #8558983 - Attachment is obsolete: true
Attachment #8560279 - Flags: review?(fabrice)
Comment on attachment 8560279 [details] [diff] [review]
cache hasPendingMessage in content process

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

lgtm, but I would like QA to run a smoketest pass with that patch since there is potential for bustage.
Attachment #8560279 - Flags: review?(fabrice) → review+
qawanted to run a smoketest with the patch applied, thanks!
Keywords: qawanted
No longer blocks: 1086963
Flags: needinfo?(atsai)
Tested with local build, and I didn't encounter any serious failure. Also passed smoke tests.

Gaia      2bef75b
Gecko     74f80ff

remove qawanted and ni
Flags: needinfo?(atsai)
Keywords: qawanted
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e60f5fee730f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8560279 [details] [diff] [review]
cache hasPendingMessage in content process

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: slower app-start-up time when apps use system-message hasPendingMessages()
Testing completed: patches has been stuck on m-c for a while
Risk to taking this patch (and alternatives if risky): app may fail to detect there are pending messages thus display a wrong first page then display the correct one. Alternatively if we don't uplift this patch then apps could be changed to use window.location to detect if it's receiving system-messages
String or UUID changes made by this patch: N/A
Attachment #8560279 - Flags: approval-mozilla-b2g37?
Attachment #8560279 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Bug 1112989 needed to be backed out and this was blocking it. Backed out as well.
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5481f6e66c8b
Attached patch Patch for v2.2Splinter Review
Flags: needinfo?(kchen)
Keywords: checkin-needed
Depends on: 1117484
Depends on: 1140325
You need to log in before you can comment on or make changes to this bug.