1171013 - Loading Webapps.jsm delays startup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Eugen Sawin [:esawin]]

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.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1171013-Add-proxy-object-for-on-demand-loading-o.patch

With this patch we load Webapps.jsm only when it's used and provide a virtual proxy object on startup instead.

Comment 2

[Eugen Sawin [:esawin]]

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)

Comment 3

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1171013-Set-Android-apps-to-be-launchable-by-def.patch

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.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1171013-Add-defineModuleProxyGetter-to-XPCOMUtil.patch

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.

Comment 5

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1171013-Use-defineModuleProxyGetter-to-import-We.patch

And finally, we apply the new proxy getter to lazily import Webapps.jsm.

Comment 6

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1171013-Extend-defineLazyModuleGetter-to-support.patch

Merged the new getter functionality with defineLazyModuleGetter since it is only a minor extension of it.

Comment 7

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1171013-Use-extended-defineLazyModuleGetter-to-i.patch

Removed redundant service event listener.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 9

[[:fabrice] Fabrice Desré]

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.

Comment 10

[Eugen Sawin [:esawin]]

(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 11

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 12

[Eugen Sawin [:esawin]]

(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.

Comment 13

[Eugen Sawin [:esawin]]

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

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/336d4a1f0f3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed8af79d301e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba34e5e4111a

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/336d4a1f0f3c
https://hg.mozilla.org/mozilla-central/rev/ed8af79d301e
https://hg.mozilla.org/mozilla-central/rev/ba34e5e4111a

Comment 16

[Bob Clary [:bc] (inactive)]

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

Comment 17

[Richard Newman [:rnewman]]

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.