Closed Bug 834999 Opened 7 years ago Closed 7 years ago

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

Categories

(Core Graveyard :: DOM: Apps, defect)

x86_64
Linux
defect
Not set

Tracking

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18+ fixed, b2g18-v1.0.0 fixed, b2g18-v1.0.1 fixed)

RESOLVED FIXED
mozilla21
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 + fixed
b2g18-v1.0.0 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: cjones, Assigned: fabrice)

References

Details

(Keywords: perf, Whiteboard: [FFOS_perf][qa-])

Attachments

(1 file, 1 obsolete file)

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?
Assignee: nobody → fabrice
Attached patch patch v1 (obsolete) — Splinter Review
With this patch I get around 300ms of speedup when loading the system app and the homescreen.
Attachment #706748 - Flags: feedback?(jones.chris.g)
Comment on attachment 706748 [details] [diff] [review]
patch v1

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

Nice! :)
Attachment #706748 - Flags: feedback?(jones.chris.g) → feedback+
Attached patch patch v2Splinter Review
Minor cleanups to v1.
Attachment #706748 - Attachment is obsolete: true
Attachment #706756 - Flags: review?(ferjmoreno)
Whiteboard: [FFOS_perf]
blocking-b2g: --- → tef?
Fabrice, please provide more information about the benefits of this change.
Attachment #706756 - Flags: review?(ferjmoreno) → review+
(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.
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.
blocking-b2g: tef? → -
tracking-b2g18: --- → +
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 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.
Attachment #706756 - Flags: approval-mozilla-b2g18?
Attachment #706756 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Keywords: perf
https://hg.mozilla.org/releases/mozilla-b2g18/rev/10d3bf4d172c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
blocking-b2g: - → tef+
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é <fabrice@mozilla.com>
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).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Can we back this out then?
wait, I can push a followup
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/db2b28f5782c
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/78de332182cb
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Whiteboard: [FFOS_perf] → [FFOS_perf][qa-]
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.
Let's follow up at bug 837572.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.