Closed Bug 1171013 Opened 5 years ago Closed 5 years ago

Loading Webapps.jsm delays startup

Categories

(Firefox for Android :: General, defect)

41 Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: esawin, Assigned: esawin)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(3 files, 3 obsolete files)

Measured on a Nexus 5 Android 5.1, we spend ~500ms in browser.js BrowserApp.startup where loading and initializing Webapps.jsm contributes with ~180ms to it.

We can't delay loading it, because web apps and sites (like https://marketplace.firefox.com) may depend on DOMApplicationRegistry being loaded. However, we can provide a low-profile virtual proxy object which loads and initializes the heavy registry object on demand.
Assignee: nobody → esawin
Status: NEW → ASSIGNED
Keywords: perf
With this patch we load Webapps.jsm only when it's used and provide a virtual proxy object on startup instead.
The results seem to support the startup improvement (~140ms): http://phonedash.mozilla.org/#/org.mozilla.fennec/totalthrobber/local-blank/norejected/2015-06-09/2015-06-09/notcached/noerrorbars/standarderror/try (baseline: c0ecb7d75625, patched: cd0552f562b2)
Attachment #8617396 - Flags: review?(nalexander)
Attachment #8617396 - Flags: review?(nalexander) → review-
Attachment #8617396 - Flags: review-
This is an optional change, please let me know if there is an issue with handling it that way instead of setting DOMApplicationRegistry.allAppsLaunchable = true explicitly for Android.
Attachment #8617396 - Attachment is obsolete: true
Attachment #8621256 - Flags: review?(dtownsend)
This patch adds an extended version of defineLazyModuleGetter which allows the caller to specify a proxy object with corresponding setup and teardown functions, which acts on behalf of the module to be imported as long as its implementation is not required.
Attachment #8621258 - Flags: review?(mark.finkle)
And finally, we apply the new proxy getter to lazily import Webapps.jsm.
Attachment #8621260 - Flags: review?(mark.finkle)
Merged the new getter functionality with defineLazyModuleGetter since it is only a minor extension of it.
Attachment #8621258 - Attachment is obsolete: true
Attachment #8621258 - Flags: review?(mark.finkle)
Attachment #8621563 - Flags: review?(mark.finkle)
Removed redundant service event listener.
Attachment #8621260 - Attachment is obsolete: true
Attachment #8621260 - Flags: review?(mark.finkle)
Attachment #8621564 - Flags: review?(mark.finkle)
Attachment #8621256 - Flags: review?(dtownsend) → review+
Comment on attachment 8621563 [details] [diff] [review]
0002-Bug-1171013-Extend-defineLazyModuleGetter-to-support.patch

LGTM

These changes deserve a post to dev.platform once they are reviewed
Attachment #8621563 - Flags: review?(mark.finkle) → review+
I'm not super happy with the duplication of the message list in Webapps.jsm and in the proxy. I guarantee that at some point we'll forget to keep them in sync. At least add comments in both files to indicate that they need to be similar.
(In reply to Fabrice Desré [:fabrice] from comment #9)
> I'm not super happy with the duplication of the message list in Webapps.jsm
> and in the proxy. I guarantee that at some point we'll forget to keep them
> in sync. At least add comments in both files to indicate that they need to
> be similar.

Agreed, I will add a comment. Alternatively, we could move that out into a separate file, which both, Webapps.jsm and browser.js, import, but I tried not to introduce new files for this.
Comment on attachment 8621564 [details] [diff] [review]
0003-Bug-1171013-Use-extended-defineLazyModuleGetter-to-i.patch

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+XPCOMUtils.defineLazyModuleGetter(
>+  this,
>+  "DOMApplicationRegistry",
>+  "resource://gre/modules/Webapps.jsm",
>+  null,
>+  function() {
>+    XPCOMUtils.defineLazyServiceGetter(this, "ppmm",
>+                                   "@mozilla.org/parentprocessmessagemanager;1",
>+                                   "nsIMessageBroadcaster");
>+
>+    this.messages = ["Webapps:Install",

Agree about adding a comment for the duplicate messages. File a followup so we can track it forward?

Also, | this | here is the proxy object, right? Doesn't the Pre and Post lambda get the proxy object passed in as a param? Maybe you could use the proxy object param instead of | this |?

>+  function() {
>+    this.messages.forEach(msgName => {
>+      this.ppmm.removeMessageListener(msgName, this);
>+    });
>+  },

Same here for | this | ?

>+  {
>+    receiveMessage: function() {
>+      return DOMApplicationRegistry.receiveMessage.apply(

Add a simple comment here to just let people know this proxy object is just forwarding messages to the real object?
(In reply to Mark Finkle (:mfinkle) from comment #11)
> Agree about adding a comment for the duplicate messages. File a followup so
> we can track it forward?

I'll add the comment in both places (Webapps.jsm and browser.js) to keep the messages in sync.

> Also, | this | here is the proxy object, right? Doesn't the Pre and Post
> lambda get the proxy object passed in as a param? Maybe you could use the
> proxy object param instead of | this |?

We set |this| to the proxy object via the first |apply| argument, it's not passed as a parameter.

> Add a simple comment here to just let people know this proxy object is just
> forwarding messages to the real object?

Will do. Also this does not merely forward the message, it implicitly triggers its import, too.
Attachment #8621564 - Flags: review?(mark.finkle) → review+
Depends on: 1175605
caused bug 1175605. can we back this out?

If you need to test this in try, use try: -b o -p android-api-9,android-api-11 -u autophone-webapp -t none
This is a huge perf win, so I think we'd all very much rather fix the autophone tests than back this out. A middle ground, I think, is for Eugen to fix the test :D

Commented as such in Bug 1175605.
You need to log in before you can comment on or make changes to this bug.