Closed
Bug 834999
Opened 8 years ago
Closed 8 years ago
WebappsApplication.prototype.manifest getter takes 25-30ms on critical startup path
Categories
(Core Graveyard :: DOM: Apps, defect)
Tracking
(blocking-b2g:tef+, 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)
3.37 KB,
patch
|
ferjm
:
review+
milan
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•8 years ago
|
||
Profile is http://people.mozilla.com/~bgirard/cleopatra/#report=38fcb0169ca5f710c80508b04e5a37dcd4446d2d , but you have to be able to see the right view.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → fabrice
Assignee | ||
Comment 2•8 years ago
|
||
With this patch I get around 300ms of speedup when loading the system app and the homescreen.
Attachment #706748 -
Flags: feedback?(jones.chris.g)
Reporter | ||
Comment 3•8 years ago
|
||
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•8 years ago
|
||
Minor cleanups to v1.
Attachment #706748 -
Attachment is obsolete: true
Attachment #706756 -
Flags: review?(ferjmoreno)
Updated•8 years ago
|
Whiteboard: [FFOS_perf]
Assignee | ||
Updated•8 years ago
|
blocking-b2g: --- → tef?
Comment 5•8 years ago
|
||
Fabrice, please provide more information about the benefits of this change.
Updated•8 years ago
|
Attachment #706756 -
Flags: review?(ferjmoreno) → review+
Assignee | ||
Comment 6•8 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.
Comment 7•8 years ago
|
||
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:
--- → +
Assignee | ||
Comment 8•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8d1949eeb05
Comment 9•8 years ago
|
||
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 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/aea13422f063
Assignee | ||
Comment 11•8 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?
Updated•8 years ago
|
Attachment #706756 -
Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Assignee | ||
Comment 12•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/10d3bf4d172c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 13•8 years ago
|
||
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.
Updated•8 years ago
|
blocking-b2g: - → tef+
status-b2g18:
--- → fixed
Comment 16•8 years ago
|
||
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 → ---
Comment 17•8 years ago
|
||
Can we back this out then?
Reporter | ||
Comment 18•8 years ago
|
||
Please back out.
Assignee | ||
Comment 19•8 years ago
|
||
wait, I can push a followup
Assignee | ||
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e4fc8131668
Assignee | ||
Comment 21•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/0e0d149b509a
Reporter | ||
Comment 22•8 years ago
|
||
Fix -> m-c at fabrice's request https://hg.mozilla.org/mozilla-central/rev/50cf5bbcb180
Comment 23•8 years ago
|
||
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: 8 years ago → 8 years ago
status-firefox19:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Updated•8 years ago
|
Whiteboard: [FFOS_perf] → [FFOS_perf][qa-]
Comment 25•8 years ago
|
||
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•8 years ago
|
||
Let's follow up at bug 837572.
Updated•8 years ago
|
status-b2g18-v1.0.1:
--- → fixed
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•