I have played a bit more with preloading and this patch is the result of my investigation. I don't think I can preload more things actually and the other things that affects the load time of the template app seems out of the preloading path. This patch loads more services related to some specifics apps: - + /* Applications Specific Helper */ Cc["@mozilla.org/alarmsManager;1"].getService(Ci["nsIDOMMozAlarmsManager"]); Cc["@mozilla.org/contactManager;1"].getService(Ci["nsIDOMContactManager"]); Cc["@mozilla.org/settingsManager;1"].getService(Ci["nsIDOMSettingsManager"]); Cc["@mozilla.org/system-message-manager;1"].getService(Ci["nsIDOMNavigatorSystemMessages"]); Is there any secuirty issues of preloading those here? The patch preloads the ContentSecurityPolicy loaded by privileged/certified apps: Cc["@mozilla.org/contentsecuritypolicy;1"].createInstance(Ci["nsIContentSecurityPolicy"]); The patch loads one more additional service for app that use the console API: - Cc["@mozilla.org/console-api;1"].createInstance(Ci["nsISupports"]); Also the patch do less call to messageManager.loadFrameScript (that are done once TabChild is connected to the parent) and more mozIJSSubscriptLoader.loadFrameScript when possible. And the last thing beeing that a big part of the BrowserElementChild logic has been moved to be preloaded if needed. The things that seems to stays between the click on the homescreen and the mozbrowserloadend of the template app seems out of the scope of this bug: - Time to sent the message from the user click on the homescreen to the system app in order to open the app: ~20ms (mostly related to webapps.jsm) - Time to append to the dom an <iframe mozbrowser mozapp>: ~40ms (half of the time is when calling: new BrowserElementParent() in BrowserElementParent.js.) - Random time in Gaia spent doing 'something': ~40ms - Time between the loadstart/loadend of the template app: ~130ms (It seems like the JS web progress listeners on the BrowserElementChild side are a non neglige able part of the time here). My measurements are made with the animation disabled when opening an application since there is still a regression on it which is out of the scope of this work again. I have also some local patches applied to my Gaia tree that I need to upstream (which is why there is 'only' 40ms spent in Gaia.

I will give more numbers on how much it shaves once Etienne is ready with the future |make perfs-tests| target in Gaia :) (Hopefully tonight)

Comment on attachment 707616 [details] [diff] [review] Patch Vivien, this patch conflicts heavily with bug 835548 and bug 835584, which are known wins validated by the profiler and stopwatch. Can you work with fabrice to figure out which patch hunks we should take from which?

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #2) > I will give more numbers on how much it shaves once Etienne is ready with > the future |make perfs-tests| target in Gaia :) (Hopefully tonight) If you're running black-box tests, make sure they're measuring pixels appearing on screen. Otherwise, verify your wins by using a tool like the gecko profiler to show in before/after profiles that you've removed significant amount of work. We have too many people going down dead ends by trying to optimize the firing of DOM events.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4) > (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #2) > > I will give more numbers on how much it shaves once Etienne is ready with > > the future |make perfs-tests| target in Gaia :) (Hopefully tonight) > > If you're running black-box tests, make sure they're measuring pixels > appearing on screen. Otherwise, verify your wins by using a tool like the > gecko profiler to show in before/after profiles that you've removed > significant amount of work. > > We have too many people going down dead ends by trying to optimize the > firing of DOM events. (In reply to Chris Jones [:cjones] [:warhammer] from comment #3) > Comment on attachment 707616 [details] [diff] [review] > Patch > > Vivien, this patch conflicts heavily with bug 835548 and bug 835584, which > are known wins validated by the profiler and stopwatch. Can you work with > fabrice to figure out which patch hunks we should take from which? Sure. It seems like bug 835548 is compatible with this one actually. Fabrice works mostly on BrowserElementParent while I worked mostly on BrowserElementChild. \o/ About bug 835584 (so your patch) it seems like we collapse a lot. Both approach make sense but I move the execution of those scripts on the prelaunch process while if I read correctly your code you 'just' move the compile phase on the side of the prelaunch process (it seems like you will also call UserAgentOverrides.init(); twice since it will be inserted by mm.loadFrameScript as well) I guess I need to see with you for this one? For what it worth here are the numbers I have with the new |make perf-test| target in Gaia (still in the perfs branch of Etienne gibhub repository): Without the patch: === Gallery average load time: 1309.4ms === Dialer average load time: 1304.8ms === Contacts average load time: 2729.2ms === SMS average load time: 1545.8ms === Clock average load time: 1530.2ms With the patch: === Gallery average load time: 1191ms === Dialer average load time: 1211.4ms === Contacts average load time: 2606.2ms === SMS average load time: 1380ms === Clock average load time: 1305.4ms So it seems a 100ms win and sometimes more. We mid-air-collisioned on the comment so I didn't seen the comment you made about DOM Events... I rely on it for measuring but the real test I use is side to side device.

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #5) > About bug 835584 (so your patch) it seems like we collapse a lot. Both > approach make sense but I move the execution of those scripts on the > prelaunch process while if I read correctly your code you 'just' move the > compile phase on the side of the prelaunch process (it seems like you will > also call UserAgentOverrides.init(); twice since it will be inserted by > mm.loadFrameScript as well) There were two big overheads removed there: compiling forms.js and compiling+executing UserAgentOverrides.jsm. We can't eagerly execute forms.js because it requires a valid tab context. The problem with eagerly executing UserAgentOverrides.jsm is that it breaks the product-specific UAO_child.js hack; we're not supposed to process the overrides unless they're enabled for the product. (I *really* dislike that hack, fwiw.) So once we figure out a way around that I don't care which bug we land that improvement in. We can eagerly execute UAO_child.js too, because it doesn't depend on having a tab context. But we still have the product-specific enabled-ness issue.

(My preference would be to remove UAO_child.js in favor of a more feature-centric solution, rather than product-centric. For example, checking to see if any of the override.* prefs exist and executing UserAgentOverrides.jsm if so.)

Comment on attachment 707616 [details] [diff] [review] Patch Testing with Template and gecko cedbc43dfa7602f37ba451d2bdb6c3536fea823b / gaia d6595fbdd9cd8de0c169be57bc11aaebc1849dd7 before: 0.84, 0.97, 0.97, 0.87, 0.91 after: 0.81, 0.79, 0.82, 0.81, 0.85

Comment on attachment 707616 [details] [diff] [review] Patch r=me, but there are a few things I'd like to clear up before we land this. >diff --git a/dom/browser-element/BrowserElementChild.js b/dom/browser-element/BrowserElementChild.js >--- a/dom/browser-element/BrowserElementChild.js >+++ b/dom/browser-element/BrowserElementChild.js > function debug(msg) { >- //dump("BrowserElementChild - " + msg + "\n"); >+ dump("BrowserElementChild - " + msg + "\n"); > } Please revert this change. >+var isReady = true; I'd prefer if this was called BROWSER_ELEMENT_CHILD_IS_READY or something, to match 'PRELOAD'. >diff --git a/dom/browser-element/BrowserElementChild.js b/dom/browser-element/BrowserElementChildPreload.js >copy from dom/browser-element/BrowserElementChild.js >copy to dom/browser-element/BrowserElementChildPreload.js >--- a/dom/browser-element/BrowserElementChild.js >+++ b/dom/browser-element/BrowserElementChildPreload.js >@@ -1,42 +1,49 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this file, > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > >+if (!BROWSER_ELEMENT_CHILD_PRELOAD_JS) { >+var BROWSER_ELEMENT_CHILD_PRELOAD_JS = 1; I don't see how we could get into a situation where we try to load this file twice. But if that happens, don't we want that to cause an error so we can notice it? Surely loading this JS file twice would not help perf. If we really do need to protect this file against being loaded twice, I'd prefer an early return over a giant if statement, unless there's a good reason to do it like this. >+var isReady = false; >+ >+(function() { >+ > "use strict"; >+dump("######################## browserelementchildpreload.js loaded\n"); Capitalization, please. > let { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components; > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://gre/modules/Services.jsm"); >-Cu.import("resource://gre/modules/Geometry.jsm"); > Cu.import("resource://gre/modules/BrowserElementPromptService.jsm"); > > // Event whitelisted for bubbling. > let whitelistedEvents = [ > Ci.nsIDOMKeyEvent.DOM_VK_ESCAPE, // Back button. > Ci.nsIDOMKeyEvent.DOM_VK_SLEEP, // Power button. > Ci.nsIDOMKeyEvent.DOM_VK_CONTEXT_MENU, > Ci.nsIDOMKeyEvent.DOM_VK_F5, // Search button. > Ci.nsIDOMKeyEvent.DOM_VK_PAGE_UP, // Volume up. > Ci.nsIDOMKeyEvent.DOM_VK_PAGE_DOWN // Volume down. > ]; > > function debug(msg) { >- //dump("BrowserElementChild - " + msg + "\n"); >+ //dump("BrowserElementChildPreload (" + Date.now() + ") - " + msg + "\n"); > } > > function sendAsyncMsg(msg, data) { >+ // Ensure to not send any message while the process is not ready. Ensure that we don't send any messages before BrowserElementChild.js finishes loading. >@@ -367,23 +362,26 @@ BrowserElementChild.prototype = { > _addMozAfterPaintHandler: function(callback) { > function onMozAfterPaint() { >+ let start = Date.now(); > let uri = docShell.QueryInterface(Ci.nsIWebNavigation).currentURI; >- debug("Got afterpaint event: " + uri.spec); > if (uri.spec != "about:blank") { >+ debug("Got afterpaint event: " + uri.spec); > removeEventListener('MozAfterPaint', onMozAfterPaint, > /* useCapture = */ true); > callback(); > } >+ debug("MozAfterPaint: " + (Date.now() - start) + "\n"); Here and elsewhere: debug() messages already have \n. It's not clear to me why you want this debug message outside the about:blank check, but if you leave it in, could you say something like "MozAfterPaint (about:blank):"? >+ let stsart = Date.now(); Looks like this was left in by accident. >@@ -739,34 +737,33 @@ BrowserElementChild.prototype = { >+ sendAsyncMsg('locationchange', Services.uriFixup.createExposableURI(location).spec); Please wrap this long line. >@@ -817,14 +814,10 @@ BrowserElementChild.prototype = { > > get messageManager() { > return this._messageManagerPublic; > } > }; > > var api = new BrowserElementChild(); > >-// FIXME/bug 775438: use a JSM? >-// >-// The code in this included file depends on the |addEventListener|, >-// |addMessageListener|, |content|, |Geometry| and |Services| symbols >-// being "exported" from here. >-#include BrowserElementScrolling.js >+}).bind(this)(); >+} >diff --git a/dom/browser-element/BrowserElementParent.js b/dom/browser-element/BrowserElementParent.js >--- a/dom/browser-element/BrowserElementParent.js >+++ b/dom/browser-element/BrowserElementParent.js >+ function attachMessages() { attachMessageListeners(), please. >+ } >+ >+ // Once the tabchild emit an hello message start listening for more >+ // messages. There is nothing interesting before. >+ addMessageListener("hello", attachMessages); // Start listening for other messages only after the BrowserElementChild // sends us a "hello" message. This allows us to avoid the overhead of // many addMessageListener() calls during BrowserElementParent startup. It's not clear to me why this is a win, because the 'hello' message comes pretty early. But in any case, you need to harmonize this with Fabrice's patch (which I think has a better approach for this). >@@ -369,25 +374,22 @@ BrowserElementParent.prototype = { > > // Inform our child if our owner element's document is invisible. Note > // that we must do so here, rather than in the BrowserElementParent > // constructor, because the BrowserElementChild may not be initialized when > // we run our constructor. > if (this._window.document.mozHidden) { > this._ownerVisibilityChange(); > } >+ return { >+ name: this._frameElement.getAttribute('name'), >+ fullscreen: this._frameElement.hasAttribute('allowfullscreen') || >+ this._frameElement.hasAttribute('mozallowfullscreen') "fullscreenAllowed", please. >diff --git a/dom/ipc/preload.js b/dom/ipc/preload.js >--- a/dom/ipc/preload.js >+++ b/dom/ipc/preload.js >@@ -30,16 +32,17 @@ >+ Cc["@mozilla.org/console-api;1"].createInstance(Ci["nsISupports"]); >@@ -65,15 +68,31 @@ >+ /* Applications Specific Helper */ >+ Cc["@mozilla.org/alarmsManager;1"].getService(Ci["nsIDOMMozAlarmsManager"]); >+ Cc["@mozilla.org/contactManager;1"].getService(Ci["nsIDOMContactManager"]); >+ Cc["@mozilla.org/contentsecuritypolicy;1"].createInstance(Ci["nsIContentSecurityPolicy"]); >+ Cc["@mozilla.org/settingsManager;1"].getService(Ci["nsIDOMSettingsManager"]); >+ Cc["@mozilla.org/system-message-manager;1"].getService(Ci["nsIDOMNavigatorSystemMessages"]); >+ >+ Services.scriptloader.loadSubScript("chrome://browser/content/forms.js", global); >+ Services.scriptloader.loadSubScript("chrome://browser/content/UAO_child.js", global); >+ Services.scriptloader.loadSubScript("chrome://global/content/BrowserElementPanning.js", global); >+ Services.scriptloader.loadSubScript("chrome://global/content/BrowserElementChildPreload.js", global); >+ >+ Services.io.getProtocolHandler("app"); >+ Services.io.getProtocolHandler("default"); I'm concerned about the parts of this that load services we may not need. This is a shift in strategy for us. In the past, we only preloaded things which we /knew/ we would need. This patch proposes that we preload things we are likely /not/ to need, such as the alarms manager. I'd prefer that we didn't do this unless we could show * Preloading this specific thing is a large win for the relevant app, and * Preloading this specific thing uses a negligible amount of additional memory. for each of the non-essential things we're preloading. Would you be OK splitting this part into a separate bug so we can investigate without blocking the rest of your changes? Separately, I should think that the console API ought not to be on the critical startup path for any of our apps; if it is, that sounds like a bug in the app to me. > docShell.isActive = false; > docShell.QueryInterface(Ci.nsIWebNavigation) > .loadURI("about:blank", >- Ci.nsIWebNavigation.LOAD_FLAGS_NONE, >+ Ci.nsIWebNavigation.LOAD_FLAGS_FIRST_LOAD | >+ Ci.nsIWebNavigation.STOP_ALL, > null, null, null); What's the purpose of this change?

Comment on attachment 707616 [details] [diff] [review] Patch Review of attachment 707616 [details] [diff] [review]: ----------------------------------------------------------------- feedback+ from me, but I'd like to get new numbers with that patch rebased on top of Bug 835548. ::: b2g/components/Keyboard.jsm @@ -47,5 @@ > let mm = frameLoader.messageManager; > mm.addMessageListener('Forms:Input', this); > - > - try { > - mm.loadFrameScript(kFormsFrameScript, true); Please remove also the kFormsFrameScript declaration. ::: dom/ipc/jar.mn @@ +5,5 @@ > toolkit.jar: > content/global/test-ipc.xul (test.xul) > content/global/remote-test-ipc.js (remote-test.js) > + content/global/BrowserElementChild.js (../browser-element/BrowserElementChild.js) > +* content/global/BrowserElementChildPreload.js (../browser-element/BrowserElementChildPreload.js) It looks like this file is missing from the patch

Does this just need to land?

Viven, do you have time to check if your patch still applies?

This is a rebased patch. Justin the main reason for having a guard in BrowserElementChild.js is not to prevent loading them twice but to load them if they have not been loaded by the preloaded process. Which is the case for the system app and the browser app. Addressing review comments: I removed some changes to BrowserElementParent.js (the attachMessages part). I removed also the changes to the way about:blank is loaded. I removed preloading some specifics things for applications. I kept the csp and the settings one since those are used for all privileged/certified apps (settings is used for l10n - sorry :/). Justin let me know if you don't want those in the patch as well? I'm re-requesting review because of that.

Comment on attachment 710765 [details] [diff] [review] Patch v2 Review of attachment 710765 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/preload.js @@ +76,5 @@ > + /* Applications Specific Helper */ > + Cc["@mozilla.org/settingsManager;1"].getService(Ci["nsIDOMSettingsManager"]); > + > + Services.scriptloader.loadSubScript("chrome://browser/content/forms.js", global); > + Services.scriptloader.loadSubScript("chrome://browser/content/UAO_child.js", global); We don't need this one anymore. cjones removed it.

This is still a measurable win on gecko-18 even with what's been uplifted from bug 834622, but the functionality overlaps with bug 835584 (which hasn't been uplifted). Let me see what win here there is beyond that.

Measuring with a gecko based on gecko-18 as of earlier today, it's hard to measure a stopwatch difference between the the two. But, - both patches remove the overhead from RecvLoadScript(), but this one drops it off profiles entirely - this patch very clearly removes more BEC samples from the profiles With this patch the only noticeable samples around BEC are spent blocked on sync IPC that's fixed by deps of bug 834622. Neither this patch nor the one in bug 834622 makes much difference on the parent side. I think we should - rebase this on the changes from inbound (bug 835584) and land it - start the process of uplifting this - get a version of the patch here rebased on everything from bug 834622. With the other patches there, the only significant time we're spending is in BEP/BEC, so this should help noticeably. This is less urgent. Profiles gecko-18 500d142065d5fb0d2f0c94622754836bb6e9327b b2g process http://people.mozilla.com/~bgirard/cleopatra/#report=0484c41583d493b0f96fe5a8dc1f994f9928e1c5 http://people.mozilla.com/~bgirard/cleopatra/#report=508d976f054c8022bbdd3a9c429bdd699d606a86 Template process http://people.mozilla.com/~bgirard/cleopatra/#report=584e7b23a8cf009be76e9aa205c4e36dff08d9b5 http://people.mozilla.com/~bgirard/cleopatra/#report=006265c2445c64eaa7b2984e5eda2fce012e044e 500d142065d5fb0d2f0c94622754836bb6e9327b with bug 835584 b2g process http://people.mozilla.com/~bgirard/cleopatra/#report=8171610ff4ee066a3f8204e8ffb4bfc5ab93db41 http://people.mozilla.com/~bgirard/cleopatra/#report=22ef4dc081b3b1ebf9b49bdaec0e4b2e2c572c5b Template process http://people.mozilla.com/~bgirard/cleopatra/#report=3a2a0ff461d6bee935a6ef269fce374fcaeaa818 http://people.mozilla.com/~bgirard/cleopatra/#report=afd644764fefc71d4ae30c332fef2d33604e641c 500d142065d5fb0d2f0c94622754836bb6e9327b with patch here b2g process http://people.mozilla.com/~bgirard/cleopatra/#report=5685a1529369cc77fa943b499331be02218c0b01 http://people.mozilla.com/~bgirard/cleopatra/#report=da3aa2218297598a90784cd4559287c8ce3458b5 Template process http://people.mozilla.com/~bgirard/cleopatra/#report=51b119c540cce14ba0b955cd744a69d73c08cc0e http://people.mozilla.com/~bgirard/cleopatra/#report=5e184802a8818c7327926c581217cecb464dc474

> Let me know if you don't want those in the patch as well? I'd prefer that we loaded only those things that * we know we'll need, or * we might need, and that we can show have a low impact on memory usage and a measurable positive impact on app startup speed. > The main reason for having a guard in BrowserElementChild.js is not to prevent > loading them twice but to load them if they have not been loaded by the preloaded process. Okay, but could we guard outside BEC.js? That way we don't load all of BEC.js just to decide we're not going to run it? I don't gate my review on either of these things.

This patch is rebased on the latest b2g-18. I kept the settings and content policy because it makes a different in my tests for the core apps.

(In reply to Justin Lebar [:jlebar] from comment #17) > > Let me know if you don't want those in the patch as well? > > I'd prefer that we loaded only those things that > > * we know we'll need, or > * we might need, and that we can show have a low impact on memory usage and > a measurable positive impact on app startup speed. I did not really measure the memory impact on memory tbh. > > The main reason for having a guard in BrowserElementChild.js is not to prevent > > loading them twice but to load them if they have not been loaded by the preloaded process. > > Okay, but could we guard outside BEC.js? That way we don't load all of > BEC.js just to decide we're not going to run it? > BEC.js is really small now. The guard is just to not load the dependencies. That would be resolved elegantly with Harmony Modules at some points ;)

The startup profile looks so pretty now. Majority of the samples are spent blocked on sync IPC; using almost no CPU cycles running code unrelated to loading the app itself. http://people.mozilla.com/~bgirard/cleopatra/#report=f07305710978d940cfe545076da587b87692f792

The rebase on inbound was pretty nontrivial, hope the test coverage is good ...

Chrome file doesn't exist: /home/cjones/mozilla/iff-dbg/dist/bin/chrome/browser/content/browser/forms.js JavaScript error: , line 0: uncaught exception: Error opening input stream (invalid filename?)

Lamesauce ... All the browser-element tests pass with this, taking a last spin on device.

For when things are looking good upstream.

(Not regularly scheduled accelerated uplift, but this is a rot-happy patch that I need for something tomorrow.) https://hg.mozilla.org/releases/mozilla-b2g18/rev/71ddeff45ec2

(In reply to Chris Jones [:cjones] [:warhammer] from comment #21) > Created attachment 711548 [details] [diff] [review] > Remove code from bug 835584 obsoleted by this > > The startup profile looks so pretty now. Majority of the samples are spent > blocked on sync IPC; using almost no CPU cycles running code unrelated to > loading the app itself. > > http://people.mozilla.com/~bgirard/cleopatra/ > #report=f07305710978d940cfe545076da587b87692f792 Looking at the profile you linked I have opened 2 followups: - https://bugzilla.mozilla.org/show_bug.cgi?id=839473 - https://bugzilla.mozilla.org/show_bug.cgi?id=839472

Chris, this is marked as tef+, but it doesn't apply cleanly to b2g18_v1_0_0 due to bug 835584 being only on b2g18. Can you post a branch-specific patch or handle the uplift?

Yes, I'll take care of it.

Batch edit: bugs fixed on b2g18 since 1/25 branch of v1.0 are fixed on v1.0.1

Re: comment 32 - Chris did this get on to v1.0.0 and does it need to now that tef is taking v1.0.1? Please update the status flags as needed to reflect the results here.

We don't need this on 1.0.0 unless someone knows of a downstream tracking that version. (I don't know of one.)

In that case marking wontfix on v1.0.0 and if people are looking for things that didn't make it to v1.0.0 they can check that query.

Does not make sense to create a regression issue.

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Nothing to verify here.