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

RESOLVED FIXED in Firefox 21

Status

RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: cjones, Assigned: fabrice)

Tracking

({perf})

unspecified
mozilla21
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [FFOS_perf][qa-])

Attachments

(1 attachment, 1 obsolete attachment)

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)

Updated

6 years ago
Assignee: nobody → fabrice
(Assignee)

Comment 2

6 years ago
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.
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+
(Assignee)

Comment 4

6 years ago
Created attachment 706756 [details] [diff] [review]
patch v2

Minor cleanups to v1.
Attachment #706748 - Attachment is obsolete: true
Attachment #706756 - Flags: review?(ferjmoreno)
Whiteboard: [FFOS_perf]
(Assignee)

Updated

6 years ago
blocking-b2g: --- → tef?
Fabrice, please provide more information about the benefits of this change.
Attachment #706756 - Flags: review?(ferjmoreno) → review+
(Assignee)

Comment 6

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
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
(Assignee)

Comment 12

6 years ago
https://hg.mozilla.org/releases/mozilla-b2g18/rev/10d3bf4d172c
Status: NEW → RESOLVED
Last Resolved: 6 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.
Ah, now I see.
status-b2g18-v1.0.0: --- → affected
blocking-b2g: - → tef+
status-b2g18: --- → fixed
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?
Please back out.
(Assignee)

Comment 19

6 years ago
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
Last Resolved: 6 years ago6 years ago
status-b2g18-v1.0.0: affected → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Updated

6 years ago
Whiteboard: [FFOS_perf] → [FFOS_perf][qa-]
Depends on: 837572
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.
status-b2g18-v1.0.1: --- → fixed

Updated

a year ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.