Regression: Firefox - Trace Malloc Allocs - CentOS (x86_64) release 5 (Final) - 2.66% increase ---------------------------------------------------------------------------------------------- Previous: avg 540981.533 stddev 3628.872 of 30 runs up to revision 8aeabe064932 New : avg 555370.600 stddev 1510.698 of 5 runs since revision bfced2ecc0cf Change : +14389.067 (2.66% / z=3.965) Graph : http://mzl.la/10de8Fu Changeset range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8aeabe064932&tochange=bfced2ecc0cf Changesets: * http://hg.mozilla.org/mozilla-central/rev/e93a4da26856 : Doug Turner <dougt@dougt.org> - Bug 822712 - SimplePush - Interface. r=dougt, sr=sicking : http://bugzilla.mozilla.org/show_bug.cgi?id=822712 * http://hg.mozilla.org/mozilla-central/rev/2aaf82b852e7 : Nikhil Marathe <nsm.nikhil@gmail.com> - Bug 822712 - SimplePush - Implementation. r=dougt, jst, jlebar : http://bugzilla.mozilla.org/show_bug.cgi?id=822712 * http://hg.mozilla.org/mozilla-central/rev/bfced2ecc0cf : Doug Turner <dougt@dougt.org> - Bug 822712 - SimplePush - UDP Wakeup feature. r=jst, jlebar : http://bugzilla.mozilla.org/show_bug.cgi?id=822712 Bugs: * http://bugzilla.mozilla.org/show_bug.cgi?id=822712 - Simple Push notifications for B2G ----------------- Regression: Firefox-Non-PGO - Tp5 No Network Row Major MozAfterPaint (Non-Main Startup File IO Bytes) - Win7 - 38% increase --------------------------------------------------------------------------------------------------------------------------- Previous: avg 560341.633 stddev 99685.221 of 30 runs up to revision 8aeabe064932 New : avg 773273.800 stddev 19911.764 of 5 runs since revision bfced2ecc0cf Change : +212932.167 (38% / z=2.136) Graph : http://mzl.la/10dgSms Changeset range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8aeabe064932&tochange=bfced2ecc0cf Changesets: * http://hg.mozilla.org/mozilla-central/rev/e93a4da26856 : Doug Turner <dougt@dougt.org> - Bug 822712 - SimplePush - Interface. r=dougt, sr=sicking : http://bugzilla.mozilla.org/show_bug.cgi?id=822712 * http://hg.mozilla.org/mozilla-central/rev/2aaf82b852e7 : Nikhil Marathe <nsm.nikhil@gmail.com> - Bug 822712 - SimplePush - Implementation. r=dougt, jst, jlebar : http://bugzilla.mozilla.org/show_bug.cgi?id=822712 * http://hg.mozilla.org/mozilla-central/rev/bfced2ecc0cf : Doug Turner <dougt@dougt.org> - Bug 822712 - SimplePush - UDP Wakeup feature. r=jst, jlebar : http://bugzilla.mozilla.org/show_bug.cgi?id=822712 Bugs: * http://bugzilla.mozilla.org/show_bug.cgi?id=822712 - Simple Push notifications for B2G ----------------- Regression: Firefox - Trace Malloc Allocs - WINNT 5.2 - 2.69% increase ---------------------------------------------------------------------- Previous: avg 452586.467 stddev 2782.522 of 30 runs up to revision 8aeabe064932 New : avg 464763.800 stddev 993.787 of 5 runs since revision bfced2ecc0cf Change : +12177.333 (2.69% / z=4.376) Graph : http://mzl.la/10dk1Cz Changeset range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8aeabe064932&tochange=bfced2ecc0cf Changesets: * http://hg.mozilla.org/mozilla-central/rev/e93a4da26856 : Doug Turner <dougt@dougt.org> - Bug 822712 - SimplePush - Interface. r=dougt, sr=sicking : http://bugzilla.mozilla.org/show_bug.cgi?id=822712 * http://hg.mozilla.org/mozilla-central/rev/2aaf82b852e7 : Nikhil Marathe <nsm.nikhil@gmail.com> - Bug 822712 - SimplePush - Implementation. r=dougt, jst, jlebar : http://bugzilla.mozilla.org/show_bug.cgi?id=822712 * http://hg.mozilla.org/mozilla-central/rev/bfced2ecc0cf : Doug Turner <dougt@dougt.org> - Bug 822712 - SimplePush - UDP Wakeup feature. r=jst, jlebar : http://bugzilla.mozilla.org/show_bug.cgi?id=822712 Bugs: * http://bugzilla.mozilla.org/show_bug.cgi?id=822712 - Simple Push notifications for B2G Fix (ideally immediately), or backout.

Tracemalloc Allocs is just a count of the calls to malloc, which isn't a particularly useful performance metric. That said I'm kind of surprised that this code runs on Windows at all ...

Okay -- let's ignore Tracemalloc allocs for now. Yes, it could be pref'ed off until we have a desktop UI.. The worry is: Regression: Firefox-Non-PGO - Tp5 No Network Row Major MozAfterPaint (Non-Main Startup File IO Bytes) - Win7 - 38% increase In the startup, we are basically looking at indexedDB. This probably should be delayed so we aren't in the startup path. Other similar components wait until browser-ui-startup-complete for this. What do you think Kyle?

Waiting for startup to complete sounds reasonable, though I'd be very surprised that this delays startup by 37% if all it's doing is kicking off some indexedDB calls.

I added timing and nothing stands out: -*- PushService.js: 1364756000823 final-ui-startup start -*- PushService.js: init() -*- PushService.js: 1364756000824 new PushDB start -*- PushService.js: PushDB() -*- PushService.js: 1364756000825 new PushDB start -*- PushService.js: 1364756000825 ppmm access start -*- PushService.js: 1364756000826 ppmm access end -*- PushService.js: 1364756000826 getallchannelids start -*- PushService.js: getAllChannelIDs() -*- PushService.js: 1364756000827 getallchannelids end -*- PushService.js: 1364756000828 final-ui-startup end

Comment on attachment 731687 [details] [diff] [review] disable pref Review of attachment 731687 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/push/src/PushService.js @@ +280,5 @@ > > observe: function observe(aSubject, aTopic, aData) { > switch (aTopic) { > case "app-startup": > + if (this._prefs.get("enabled") == false) { When there is no existing pref, _prefs.get() will return undefined unless a default value is passed. So this fails on FF Nightly and PushService still starts. How about: if (!this._prefs.get("enabled")) @@ +281,5 @@ > observe: function observe(aSubject, aTopic, aData) { > switch (aTopic) { > case "app-startup": > + if (this._prefs.get("enabled") == false) { > + return; Nit: Indent is 2 spaces.

Updated patch which adds a (false by default) pref to all platforms and checks for pref in navigator.push as well.

Comment on attachment 732115 [details] [diff] [review] disable pref Review of attachment 732115 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

My testing clearly wasn't correct. It looks like this test: if (!this._prefs.get("enabled")) during app startup was always false no matter what value the preference was. What follows is a patch to move away from the getter on PushService and just to make a const. This is what other services do.

(In reply to Doug Turner (:dougt) from comment #11) > My testing clearly wasn't correct. It looks like this test: > > if (!this._prefs.get("enabled")) > > during app startup was always false no matter what value the preference was. This happens surprisingly often - I had filed bug 814517 for this issue a while back. Perhaps a system-wide check for this sort of thing would be a good idea.

Comment on attachment 735011 [details] [diff] [review] patch v.2 follow up Review of attachment 735011 [details] [diff] [review]: ----------------------------------------------------------------- Landing changed patch with whitespace nits fixed and sandbox bits removed. ::: dom/push/src/PushService.js @@ +19,5 @@ > Cu.import("resource://gre/modules/Timer.jsm"); > Cu.import("resource://gre/modules/services-common/preferences.js"); > Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js"); > > Cu.import("resource://gre/modules/identity/Sandbox.jsm"); looks like you're patching over the sandbox patch which hasn't landed yet. @@ +317,5 @@ > > observe: function observe(aSubject, aTopic, aData) { > switch (aTopic) { > case "app-startup": > + whitespace

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): 822712 User impact if declined: None Testing completed: Yes Risk to taking this patch (and alternatives if risky): Very low. Simply uses an alternative preferences object and disables push by default on all platforms except B2G. String or UUID changes made by this patch: None

Comment on attachment 741137 [details] [diff] [review] Combined fix for b2g18. Review of attachment 741137 [details] [diff] [review]: ----------------------------------------------------------------- r+ and a+. this is a required fix for b2g18 for tef+ ::: dom/push/src/Push.js @@ +39,5 @@ > init: function(aWindow) { > debug("init()"); > > + if (!Services.prefs.getBoolPref("services.push.enabled")) > + return null; isn't the style in this file: if () { stmt; } ::: dom/push/src/PushService.js @@ +281,5 @@ > observe: function observe(aSubject, aTopic, aData) { > switch (aTopic) { > case "app-startup": > + > + if (!prefs.get("enabled")) I think we need to move this into final-ui-startup.

just kidding about tef+. I meant leo+.

(In reply to Doug Turner (:dougt) from comment #19) > Comment on attachment 741137 [details] [diff] [review] > Combined fix for b2g18. > > Review of attachment 741137 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ and a+. this is a required fix for b2g18 for tef+ > > ::: dom/push/src/Push.js > @@ +39,5 @@ > > init: function(aWindow) { > > debug("init()"); > > > > + if (!Services.prefs.getBoolPref("services.push.enabled")) > > + return null; > > isn't the style in this file: > > if () { > stmt; > } I've been on the 'single statement does not require braces' style in this file. Keeping it this way. > > ::: dom/push/src/PushService.js > @@ +281,5 @@ > > observe: function observe(aSubject, aTopic, aData) { > > switch (aTopic) { > > case "app-startup": > > + > > + if (!prefs.get("enabled")) > > I think we need to move this into final-ui-startup. But that'll mean we still end up listening for 3 events, and this is *per child process* until bug 863598 lands. Putting it here we skip those.

Landed with parentheses nit fixed. The change to global preferences object in patch v.2 fixed pref not being available in app-startup, so I haven't changed that. https://hg.mozilla.org/releases/mozilla-b2g18/rev/4ad9ab79e0a0