834999 - WebappsApplication.prototype.manifest getter takes 25-30ms on critical startup path

Status: RESOLVED FIXED

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This code

  get manifest() {
    return this.manifest = ObjectWrapper.wrap(this._manifest, this._window);
  },

I see this time consumed in what looks like two separate invocations of the getter.

Anything we can speed up here?

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Profile is http://people.mozilla.com/~bgirard/cleopatra/#report=38fcb0169ca5f710c80508b04e5a37dcd4446d2d , but you have to be able to see the right view.

Comment 2

[[:fabrice] Fabrice Desré]

Created attachment 706748 [details] [diff] [review]
patch v1

With this patch I get around 300ms of speedup when loading the system app and the homescreen.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 706748 [details] [diff] [review]
patch v1

This removes manifest "stuff" from the profile almost entirely (-> 2 samples).

Nice! :)

Comment 4

[[:fabrice] Fabrice Desré]

Created attachment 706756 [details] [diff] [review]
patch v2

Minor cleanups to v1.

Comment 5

[Andrew Overholt [:overholt]]

Fabrice, please provide more information about the benefits of this change.

Comment 6

[[:fabrice] Fabrice Desré]

(In reply to Andrew Overholt [:overholt] from comment #5)
> Fabrice, please provide more information about the benefits of this change.

When exposing the app json manifest from chrome to content, we have to wrap it, and this is quite expensive. Currently even if we have several instances of app objects that represent the same app, we are rewrapping the same manifest for each instance. That happens a lot in the system app and the homescreen. This patch provides a per-manifestURL cache so we only wrap once each app manifest in a given process.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Looks good, please nominate for approval once this has been landed & tested on mainline.  We won't block on this but will take a low-risk uplift.

Comment 8

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8d1949eeb05

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-4 failures caused by something in this push.
https://hg.mozilla.org/integration/mozilla-inbound/rev/220ee1b126c3

https://tbpl.mozilla.org/php/getParsedLog.php?id=19298749&tree=Mozilla-Inbound

206 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/webapps/test_bug_779982.html | Test timed out.

Comment 10

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aea13422f063

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 706756 [details] [diff] [review]
patch v2

[Approval Request Comment]
That's a nice performance win with a simple patch that is not risky. All green on inbound.

Comment 12

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/10d3bf4d172c

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I don't know how to say it with flags, but we need to keep this on the list for v1.0.0 or approve and uplift now.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Ah, now I see.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aea13422f063

Comment 16

[Antonio Manuel Amaya Calvo (:amac)]

For some reason, line 352 on my just pulled and updated Webapps.js (mozilla-b2g18) reads:

352:    dump("-- manifestCache::evict() " + aManifest + "\n");

which besides being a non optional log is incorrect since aManifest doesn't exist. And so the package installation is broken again. 

hg log show this as being to blame:

changeset:   118400:10d3bf4d172c
user:        Fabrice Desré <[e-mail]>
date:        Thu Jan 31 10:51:22 2013 -0800
summary:     Bug 834999 - WebappsApplication.prototype.manifest getter takes 25-30ms on critical startup path r=ferjm a=milan

My m-c tree though looks fine (except for the dumps which I don't know if are intentional or not).

Comment 17

[Jason Smith [:jsmith]]

Can we back this out then?

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Please back out.

Comment 19

[[:fabrice] Fabrice Desré]

wait, I can push a followup

Comment 20

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e4fc8131668

Comment 21

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0e0d149b509a

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Fix -> m-c at fabrice's request

https://hg.mozilla.org/mozilla-central/rev/50cf5bbcb180

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/db2b28f5782c
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/78de332182cb

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e4fc8131668

Comment 25

[Gene Lian (I already quit Mozilla)]

Sorry for commenting in the resolved bug but there's a big question to me regarding the following part (I could be wrong and please correct me):

    uninit: function() {
      this._onprogress = null;
      cpmm.sendAsyncMessage("Webapps:UnregisterForMessages",
                            ["Webapps:OfflineCache",
                             "Webapps:PackageEvent",
                             "Webapps:CheckForUpdate:Return:OK"]);
 +    manifestCache.clear();
    },

This works when a inner window is destroyed. However, what's happening when we use the task manager to directly kill the app? The .uninit() won't be executed, which means the cache won't be cleared. As a result, the cached manifest might be deprecated when a new app object tries to use the cache data again.

Comment 26

[Gene Lian (I already quit Mozilla)]

Let's follow up at bug 837572.