Perform initialization tasks from idle callbacks instead of random timeouts whenever possible

RESOLVED FIXED in Firefox 57

Status

()

Firefox
General
P1
normal
RESOLVED FIXED
11 days ago
a day ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
Firefox 57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify ?

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [photon-performance])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(15 attachments)

59 bytes, text/x-review-board-request
mikedeboer
: review+
Details | Review
59 bytes, text/x-review-board-request
florian
: review+
Details | Review
59 bytes, text/x-review-board-request
florian
: review+
Details | Review
59 bytes, text/x-review-board-request
florian
: review+
Details | Review
59 bytes, text/x-review-board-request
mconley
: review+
Details | Review
59 bytes, text/x-review-board-request
florian
: review+
Details | Review
59 bytes, text/x-review-board-request
MattN
: review+
Details | Review
59 bytes, text/x-review-board-request
florian
: review+
Details | Review
59 bytes, text/x-review-board-request
MattN
: review+
Details | Review
59 bytes, text/x-review-board-request
florian
: review+
Details | Review
59 bytes, text/x-review-board-request
florian
: review+
Details | Review
59 bytes, text/x-review-board-request
florian
: review+
Details | Review
59 bytes, text/x-review-board-request
Paolo
: review+
Details | Review
59 bytes, text/x-review-board-request
jaws
: review+
Details | Review
59 bytes, text/x-review-board-request
florian
: review+
Details | Review
(Assignee)

Description

11 days ago
Let's create some structure to have a few well-defined entry points to add lazy initialization tasks that can wait. Hopefully by making it simple and easy to choose where to put them, this will be some good incentive to add more stuff to it. 
It should be well-defined enough so that the order and the timing is understood instead of feeling random.
(Assignee)

Comment 1

11 days ago
So the proposed structure is as follows: there are three new functions to use:

nsBrowserGlue.js :: _scheduleStartupIdleTasks
      browser.js :: _schedulePerWindowIdleTasks
nsBrowserGlue.js :: _scheduleArbitrarlyLateIdleTasks


The first two will run only after the notification sessionstore-windows-restored has been fired, and the last one will run at some arbitrary point in time (after a 30s user idle interval).

One advantage of moving the browser.js stuff in there is that it will run even later than _delayedStartup, only after the session has finished restoring. (And it will still run, after _delayedStartup, for new opened windows).

Also, this breaks up some of the huge startup functions that we have into various smaller functions, which should increase the chances that no single function runs for more than 16ms. And since they will be scheduled with idle callbacks, they shouldn't contribute to jank.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

11 days ago
mozreview-review
Comment on attachment 8894689 [details]
Bug 1388145 - Move CombinedStopReload.startAnimationPrefMonitoring to per-window initialization tasks.

https://reviewboard.mozilla.org/r/165848/#review170966
Attachment #8894689 - Flags: review?(jaws) → review+
Comment on attachment 8894682 [details]
Bug 1388145 - Make Master Password telemetry only run once per session (and not per window), and run it after an idle period.

https://reviewboard.mozilla.org/r/165834/#review170974
Attachment #8894682 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8894684 [details]
Bug 1388145 - Move Services.logins initialization to nsBrowserGlue.

https://reviewboard.mozilla.org/r/165838/#review170976
Attachment #8894684 - Flags: review?(MattN+bmo) → review+

Comment 20

11 days ago
mozreview-review
Comment on attachment 8894688 [details]
Bug 1388145 - Move the Downloads progress code initialization to an idle callback.

https://reviewboard.mozilla.org/r/165846/#review171124

We have a design change planned for which the Downloads button will be hidden when the browser starts up. This means that we cannot delay resuming the downloads indefinitely, because there will be no obvious way for the user to access them before the timeout, while currently they could just click on the button to have them loaded. We may even shorten the 10 seconds timeout before we check if we have data to download. So, I'm not sure we should make this change to then revert it soon.
Attachment #8894688 - Flags: review?(paolo.mozmail)

Comment 21

11 days ago
mozreview-review
Comment on attachment 8894684 [details]
Bug 1388145 - Move Services.logins initialization to nsBrowserGlue.

https://reviewboard.mozilla.org/r/165838/#review171160

::: browser/components/nsBrowserGlue.js:1154
(Diff revision 1)
> +      try {
> +        Services.logins;
> +      } catch (ex) {
> +        Cu.reportError(ex);
> +      }
> +    });

Should this keep a 3000ms timeout to match the previous behavior?

Comment 22

11 days ago
mozreview-review
Comment on attachment 8894677 [details]
Bug 1388145 - Create global and per-window entry points for scheduling initialization-related idle callbacks.

https://reviewboard.mozilla.org/r/165824/#review171140

::: browser/components/nsBrowserGlue.js:1207
(Diff revision 1)
>      E10SAccessibilityCheck.onWindowsRestored();
> +
> +    this._scheduleStartupIdleTasks();
> +
> +    this._lateTasksIdleObserver = {
> +      browserGlue: this,

Do we really need browserGlue here? Or can this be simplified to:
this._lateTasksIdleObserver = (idleService, topic, data) => {
  ...
  idleService.removeIdleObserver(this._lateTasksIdleObserver, ...
  this._scheduleArbitrarlyLateIdleTasks();
  ...
};

::: browser/components/nsBrowserGlue.js:1237
(Diff revision 1)
> +   * If you have something that can wait even further than the
> +   * per-window initialization, please schedule them using
> +   * _scheduleArbitrarlyLateIdleTasks.
> +   * Don't be fooled by thinking that the use of the timeout parameter
> +   * will delay your function: it will just ensure that it potentially
> +   * happens _earlier_ than expected (when the timeout limt has been reached),

typo: limt

::: browser/components/nsBrowserGlue.js:1254
(Diff revision 1)
> +   * LATE_TASKS_IDLE_TIME_SEC to see the current value for this idle
> +   * observer.
> +   *
> +   * Note: this function may never be called if the user is never idle for the
> +   * full length of the period of time specified. But given a reasonably low
> +   * value, this is unlikely.

Should we worry about the user becoming unidle before we are done running all the tasks scheduled from this method?
Attachment #8894677 - Flags: review?(florian) → review+

Comment 23

11 days ago
mozreview-review
Comment on attachment 8894678 [details]
Bug 1388145 - Move ContextualIdentityService and SafeBrowsing initialization to _scheduleStartupIdleTasks.

https://reviewboard.mozilla.org/r/165826/#review171184
Attachment #8894678 - Flags: review?(florian) → review+

Comment 24

11 days ago
mozreview-review
Comment on attachment 8894679 [details]
Bug 1388145 - Move the default browser check to an idle callback.

https://reviewboard.mozilla.org/r/165828/#review171144

::: commit-message-29e6c:3
(Diff revision 1)
> +Bug 1388145 - Move the default browser check to an idle callback. r=florian
> +
> +Note that the DefaultBrowserCheck.prompt() was schedule with an idle callback before and it was removed here, because it's no longer necessary (as the entire function is now running from an idle callback)

typo: schedule -> scheduled

::: browser/components/nsBrowserGlue.js:2103
(Diff revision 1)
>      Services.prefs.setIntPref("browser.migration.version", UI_VERSION);
>    },
>  
> +  _checkForDefaultBrowser() {
> +    // Perform default browser checking.
> +    if (ShellService) {

nit: return early if !ShellService, and reduce the indent on the whole method.
Attachment #8894679 - Flags: review?(florian) → review+

Comment 25

11 days ago
mozreview-review
Comment on attachment 8894681 [details]
Bug 1388145 - Move the media telemetry to an idle task.

https://reviewboard.mozilla.org/r/165832/#review171186
Attachment #8894681 - Flags: review?(florian) → review+

Comment 26

11 days ago
mozreview-review
Comment on attachment 8894683 [details]
Bug 1388145 - Make GMPInstallManager telemetry only run once per session (and not per window), and run it after an idle period.

https://reviewboard.mozilla.org/r/165836/#review171146

::: browser/components/nsBrowserGlue.js:1190
(Diff revision 1)
> +      let obj = {};
> +      Cu.import("resource://gre/modules/GMPInstallManager.jsm", obj);
> +      this._gmpInstallManager = new obj.GMPInstallManager();
> +      // We don't really care about the results, if someone is interested they
> +      // can check the log.
> +      this._gmpInstallManager.simpleCheckAndInstall().catch(() => {});

Can we / Should we add a .then(() => { delete this._gmpInstallManager; }) here? If this code is going to run only once, I don't see why we keep a reference until shutdown.
Attachment #8894683 - Flags: review?(florian) → review+

Comment 27

11 days ago
mozreview-review
Comment on attachment 8894685 [details]
Bug 1388145 - Initialize the Windows Jump Lists from an idle callback.

https://reviewboard.mozilla.org/r/165840/#review171188
Attachment #8894685 - Flags: review?(florian) → review+

Comment 28

11 days ago
mozreview-review
Comment on attachment 8894686 [details]
Bug 1388145 - Move _createExtraDefaultProfile to an idle callback.

https://reviewboard.mozilla.org/r/165842/#review171162

::: browser/components/nsBrowserGlue.js:1164
(Diff revision 1)
>      }
>  
> +    if (AppConstants.MOZ_DEV_EDITION) {
> +      Services.tm.idleDispatchToMainThread(() => {
> +        this._createExtraDefaultProfile();
> +      }

This line misses ');'
Attachment #8894686 - Flags: review?(florian) → review+

Comment 29

11 days ago
mozreview-review
Comment on attachment 8894687 [details]
Bug 1388145 - Move simple things out of delayedStartup to its proper location on _schedulePerWindowIdleTasks.

https://reviewboard.mozilla.org/r/165844/#review171166

::: browser/base/content/browser.js:1756
(Diff revision 1)
> +      // Initialize the Sync UI
> +      gSync.init();
> +    });
> +
> +    scheduleIdleTask(() => {
> +      if (AppConstants.MOZ_DATA_REPORTING) {

This test is cheap enough that I would put it outside the scheduleIdleTask call.
Attachment #8894687 - Flags: review?(florian) → review+

Comment 30

11 days ago
mozreview-review
Comment on attachment 8894690 [details]
Bug 1388145 - Move startup crashes tracking to nsBrowserGlue as it should only run once, and not per-window.

https://reviewboard.mozilla.org/r/165850/#review171170

::: browser/components/nsBrowserGlue.js:1175
(Diff revision 1)
>        this._checkForDefaultBrowser();
>      });
> +
> +    Services.tm.idleDispatchToMainThread(() => {
> +      let tmp = {};
> +      Cu.import("Timer.jsm", tmp);

Maybe:
let {setTimeout} = Cu.import("resource://gre/modules/Timer.jsm", {});

::: browser/components/nsBrowserGlue.js:1178
(Diff revision 1)
> +    Services.tm.idleDispatchToMainThread(() => {
> +      let tmp = {};
> +      Cu.import("Timer.jsm", tmp);
> +      tmp.setTimeout(function() {
> +        Services.tm.idleDispatchToMainThread(Services.startup.trackStartupCrashEnd);
> +      } , STARTUP_CRASHES_END_DELAY_MS);

nit: remove space between '}' and ','
Attachment #8894690 - Flags: review?(florian) → review+

Comment 31

11 days ago
mozreview-review
Comment on attachment 8894680 [details]
Bug 1388145 - Move the UnsubmittedCrashReporter initialization to an idle callback.

https://reviewboard.mozilla.org/r/165830/#review171190

::: browser/components/nsBrowserGlue.js:1163
(Diff revision 1)
>        SafeBrowsing.init();
>      }, 5000);
>  
> +    if (AppConstants.MOZ_CRASHREPORTER) {
> +      Services.tm.idleDispatchToMainThread(function() {
> +        UnsubmittedCrashHandler.init();

So we've moved the initialization of this thing into an idle dispatch where it wasn't before...

have you pushed this to try? I worry that we're violating this assumption in this test:

http://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/browser/modules/test/browser/browser_UnsubmittedCrashHandler.js#201-214
(In reply to :Paolo Amadini from comment #20)
> Comment on attachment 8894688 [details]
> Bug 1388145 - Move the Downloads progress code initialization to an idle
> callback.
> 
> https://reviewboard.mozilla.org/r/165846/#review171124
> 
> We have a design change planned for which the Downloads button will be
> hidden when the browser starts up. This means that we cannot delay resuming
> the downloads indefinitely, because there will be no obvious way for the
> user to access them before the timeout, while currently they could just
> click on the button to have them loaded. We may even shorten the 10 seconds
> timeout before we check if we have data to download. So, I'm not sure we
> should make this change to then revert it soon.

When is this design change expected to land? We would like to land the removal of all the timers happening during startup soon. I think the patch here is already an improvement, even if it has to be tweaked later again. If you see no problem with the patch here, I think we should take it.
Flags: needinfo?(paolo.mozmail)

Comment 33

11 days ago
mozreview-review
Comment on attachment 8894684 [details]
Bug 1388145 - Move Services.logins initialization to nsBrowserGlue.

https://reviewboard.mozilla.org/r/165838/#review171192

::: browser/components/nsBrowserGlue.js:1154
(Diff revision 1)
> +      try {
> +        Services.logins;
> +      } catch (ex) {
> +        Cu.reportError(ex);
> +      }
> +    });

Likely so. The trade-off is that if we load a webpage with a login field before this fires it will cause main-thread I/O, which is worse than the background work to load "logins.json".

This code path will perform an asynchronous initialization instead, loading "logins.json" off the main thread.

Comment 34

11 days ago
(In reply to Florian Quèze [:florian] [:flo] from comment #32)
> When is this design change expected to land?

It's an important change the UX team wants for Firefox 57. I'm not sure this is already on file as an engineering bug, though.

> We would like to land the
> removal of all the timers happening during startup soon. I think the patch
> here is already an improvement, even if it has to be tweaked later again. If
> you see no problem with the patch here, I think we should take it.

Can we set a maximum time after which we resume the downloads anyways, so we don't potentially wait indefinitely?
Flags: needinfo?(paolo.mozmail)

Updated

11 days ago
Flags: qe-verify?
Whiteboard: [photon-performance]
(In reply to :Paolo Amadini from comment #34)
> (In reply to Florian Quèze [:florian] [:flo] from comment #32)
> > When is this design change expected to land?
> 
> It's an important change the UX team wants for Firefox 57. I'm not sure this
> is already on file as an engineering bug, though.
> 
> > We would like to land the
> > removal of all the timers happening during startup soon. I think the patch
> > here is already an improvement, even if it has to be tweaked later again. If
> > you see no problem with the patch here, I think we should take it.
> 
> Can we set a maximum time after which we resume the downloads anyways, so we
> don't potentially wait indefinitely?

Sure, for now we could just keep the 10000ms value from the current setTimeout call, and use it as a timeout for the requestIdleCallback call. And it'll be easy to reduce that time in the future if needed.

Comment 36

10 days ago
mozreview-review
Comment on attachment 8894688 [details]
Bug 1388145 - Move the Downloads progress code initialization to an idle callback.

https://reviewboard.mozilla.org/r/165846/#review171242

(In reply to Florian Quèze [:florian] [:flo] from comment #35)
> Sure, for now we could just keep the 10000ms value from the current
> setTimeout call, and use it as a timeout for the requestIdleCallback call.
> And it'll be easy to reduce that time in the future if needed.

Thanks, r+ with this change.
Attachment #8894688 - Flags: review+
(Assignee)

Comment 37

10 days ago
(In reply to Florian Quèze [:florian] [:flo] from comment #22)
> Comment on attachment 8894677 [details]
> Bug 1388145 - Create global and per-window entry points for scheduling
> initialization-related idle callbacks.
> 
> https://reviewboard.mozilla.org/r/165824/#review171140
> 
> ::: browser/components/nsBrowserGlue.js:1207
> (Diff revision 1)
> >      E10SAccessibilityCheck.onWindowsRestored();
> > +
> > +    this._scheduleStartupIdleTasks();
> > +
> > +    this._lateTasksIdleObserver = {
> > +      browserGlue: this,
> 
> Do we really need browserGlue here? Or can this be simplified to:
> this._lateTasksIdleObserver = (idleService, topic, data) => {
>   ...
>   idleService.removeIdleObserver(this._lateTasksIdleObserver, ...
>   this._scheduleArbitrarlyLateIdleTasks();
>   ...
> };

done

> 
> ::: browser/components/nsBrowserGlue.js:1237
> (Diff revision 1)
> > +   * If you have something that can wait even further than the
> > +   * per-window initialization, please schedule them using
> > +   * _scheduleArbitrarlyLateIdleTasks.
> > +   * Don't be fooled by thinking that the use of the timeout parameter
> > +   * will delay your function: it will just ensure that it potentially
> > +   * happens _earlier_ than expected (when the timeout limt has been reached),
> 
> typo: limt

done

> 
> ::: browser/components/nsBrowserGlue.js:1254
> (Diff revision 1)
> > +   * LATE_TASKS_IDLE_TIME_SEC to see the current value for this idle
> > +   * observer.
> > +   *
> > +   * Note: this function may never be called if the user is never idle for the
> > +   * full length of the period of time specified. But given a reasonably low
> > +   * value, this is unlikely.
> 
> Should we worry about the user becoming unidle before we are done running
> all the tasks scheduled from this method?

No, I don't think so. As this method doesn't really run the tasks, just schedules them using idle callbacks, I don't think it would be necessary to delay things further just to wait for another long idle period. The idle callback looks reasonable enough to not pile up on anything janking.
(Assignee)

Comment 38

10 days ago
mozreview-review
Comment on attachment 8894679 [details]
Bug 1388145 - Move the default browser check to an idle callback.

https://reviewboard.mozilla.org/r/165828/#review171352
(Assignee)

Comment 39

10 days ago
mozreview-review
Comment on attachment 8894683 [details]
Bug 1388145 - Make GMPInstallManager telemetry only run once per session (and not per window), and run it after an idle period.

https://reviewboard.mozilla.org/r/165836/#review171356

::: browser/components/nsBrowserGlue.js:1190
(Diff revision 1)
> +      let obj = {};
> +      Cu.import("resource://gre/modules/GMPInstallManager.jsm", obj);
> +      this._gmpInstallManager = new obj.GMPInstallManager();
> +      // We don't really care about the results, if someone is interested they
> +      // can check the log.
> +      this._gmpInstallManager.simpleCheckAndInstall().catch(() => {});

Perhaps, but I'd like to leave that for a follow-up. The reason that was left is that `_uninit` has a `_request.abort()` call. I'm not sure what it's used for so I left it unchanged.. But looking more into it, looks like that this is actually dead code, and the `_request` object was removed at some point. In any case, let's track this in a different bug.
(Assignee)

Comment 40

10 days ago
mozreview-review
Comment on attachment 8894680 [details]
Bug 1388145 - Move the UnsubmittedCrashReporter initialization to an idle callback.

https://reviewboard.mozilla.org/r/165830/#review171430

::: browser/components/nsBrowserGlue.js:1163
(Diff revision 1)
>        SafeBrowsing.init();
>      }, 5000);
>  
> +    if (AppConstants.MOZ_CRASHREPORTER) {
> +      Services.tm.idleDispatchToMainThread(function() {
> +        UnsubmittedCrashHandler.init();

Yeah, I wasn't sure about this.. The init() function is already running from delayedStartup, so it was never guaranteed that it runs super early. So it feels like it could go together with the call to  checkForUnsubmittedCrashReports.

However.., in the interest of not introducing more changes than necessary here (to be able to track problems down more easily), I have reverted that change and will file a separate bug to see if it can be made safely.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 55

10 days ago
mozreview-review
Comment on attachment 8894680 [details]
Bug 1388145 - Move the UnsubmittedCrashReporter initialization to an idle callback.

https://reviewboard.mozilla.org/r/165830/#review171438

Thanks!

::: browser/components/nsBrowserGlue.js:1163
(Diff revision 2)
>      Services.tm.idleDispatchToMainThread(() => {
>        SafeBrowsing.init();
>      }, 5000);
>  
> +    if (AppConstants.MOZ_CRASHREPORTER) {
> +      Services.tm.idleDispatchToMainThread(function() {

Might as well use () => {} here instead of function() {}, just to fit in with the cool kids in the surrounding code.
Attachment #8894680 - Flags: review?(mconley) → review+

Comment 56

10 days ago
mozreview-review
Comment on attachment 8894676 [details]
Bug 1388145 - Add SessionStore.promiseAllWindowsRestored.

https://reviewboard.mozilla.org/r/165822/#review171672

I have a few comments, but not enough to throw in an r- ;)

If you're not comfortable landing the updated patch, please feel free to request a quick second round of review - I'll be around.

::: browser/components/sessionstore/SessionStore.jsm:553
(Diff revision 1)
>    })(),
>  
>    // Whether session has been initialized
>    _sessionInitialized: false,
>  
> +  // A promise resolved once initialization is complete

nit: missing dot.

::: browser/components/sessionstore/SessionStore.jsm:554
(Diff revision 1)
>  
>    // Whether session has been initialized
>    _sessionInitialized: false,
>  
> +  // A promise resolved once initialization is complete
> +  _deferredAllWindowsRestored: (function() {

Do you know about 'PromiseUtils.jsm'? With that, you can 'prettify' this to:
```js
_deferredAllWindowsRestored: PromiseUtils.defer(),
```

...and do the same for `_deferredInitialized` above.

Since it's such a simple abstraction, I don't really mind either way. If you're worried about adding another module: the `SessionStoreInternal` object is not frozen, so you can add lazy getters to it, if you want.

::: browser/components/sessionstore/SessionStore.jsm:4491
(Diff revision 1)
>  
>      // This was the last window restored at startup, notify observers.
>      Services.obs.notifyObservers(null,
>        this._browserSetState ? NOTIFY_BROWSER_STATE_RESTORED : NOTIFY_WINDOWS_RESTORED);
>  
> +    if (!this._browserSetState) {

I'd be even more happy if you clarify this set of conditionals by splitting up the observers notification, like:
```js
if (this._browserSetState) {
  Services.obs.notifyObservers(null, NOTIFY_BROWSER_STATE_RESTORED);
} else {
  Services.obs.notifyObservers(null, NOTIFY_WINDOWS_RESTORED);
  this._deferredAllWindowsRestored.resolve();
}
```
See what I mean?
Attachment #8894676 - Flags: review?(mdeboer) → review+
(In reply to Mike de Boer [:mikedeboer] from comment #56)

> ::: browser/components/sessionstore/SessionStore.jsm:554
> (Diff revision 1)
> >  
> >    // Whether session has been initialized
> >    _sessionInitialized: false,
> >  
> > +  // A promise resolved once initialization is complete
> > +  _deferredAllWindowsRestored: (function() {
> 
> Do you know about 'PromiseUtils.jsm'? With that, you can 'prettify' this to:
> ```js
> _deferredAllWindowsRestored: PromiseUtils.defer(),
> ```

Unfortunately that's at the cost of adding cross compartment wrappers. We just removed some PromiseUtils usage in bug 1382899 because it turned out to be expensive (in a hot code path).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 73

9 days ago
mozreview-review
Comment on attachment 8894687 [details]
Bug 1388145 - Move simple things out of delayedStartup to its proper location on _schedulePerWindowIdleTasks.

https://reviewboard.mozilla.org/r/165844/#review171802

::: browser/base/content/browser.js:1665
(Diff revision 3)
>      if (AppConstants.MOZ_DATA_REPORTING)
>        gDataNotificationInfoBar.init();
>  

The test browser_datachoices_notification.js failed when I had moved this to an idle callback, because this init() adds some observers that the test fires right after opening a new window.

Adding synchronization code just to let the test know when this is ready seems more expensive than just letting this init() here, since it just adds some observers.. So I left it here
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 100

9 days ago
All review comments addressed, now let's see how this goes on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73f82e0891f383d86e157a6fe5de6d34d8fea09e
(Assignee)

Comment 101

9 days ago
Looks good on try!

Comment 102

9 days ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7dce977f2b4e
Add SessionStore.promiseAllWindowsRestored. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/019182e62870
Create global and per-window entry points for scheduling initialization-related idle callbacks. r=florian
https://hg.mozilla.org/integration/autoland/rev/dff52943d409
Move ContextualIdentityService and SafeBrowsing initialization to _scheduleStartupIdleTasks. r=florian
https://hg.mozilla.org/integration/autoland/rev/eb129725f339
Move the default browser check to an idle callback. r=florian
https://hg.mozilla.org/integration/autoland/rev/bb80569b28ad
Move the UnsubmittedCrashReporter initialization to an idle callback. r=mconley
https://hg.mozilla.org/integration/autoland/rev/6156f27bf791
Move the media telemetry to an idle task. r=florian
https://hg.mozilla.org/integration/autoland/rev/44c5c9f4e34f
Make Master Password telemetry only run once per session (and not per window), and run it after an idle period. r=MattN
https://hg.mozilla.org/integration/autoland/rev/2db5ac710102
Make GMPInstallManager telemetry only run once per session (and not per window), and run it after an idle period. r=florian
https://hg.mozilla.org/integration/autoland/rev/4b38958eb9d2
Move Services.logins initialization to nsBrowserGlue. r=MattN
https://hg.mozilla.org/integration/autoland/rev/cd119f986edf
Initialize the Windows Jump Lists from an idle callback. r=florian
https://hg.mozilla.org/integration/autoland/rev/86c387214610
Move _createExtraDefaultProfile to an idle callback. r=florian
https://hg.mozilla.org/integration/autoland/rev/80524ce4789c
Move simple things out of delayedStartup to its proper location on _schedulePerWindowIdleTasks. r=florian
https://hg.mozilla.org/integration/autoland/rev/24b78368c9d4
Move the Downloads progress code initialization to an idle callback. r=Paolo
https://hg.mozilla.org/integration/autoland/rev/9a88d7f1d951
Move CombinedStopReload.startAnimationPrefMonitoring to per-window initialization tasks. r=jaws
https://hg.mozilla.org/integration/autoland/rev/859e1832aff0
Move startup crashes tracking to nsBrowserGlue as it should only run once, and not per-window. r=florian

Comment 103

8 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7dce977f2b4e
https://hg.mozilla.org/mozilla-central/rev/019182e62870
https://hg.mozilla.org/mozilla-central/rev/dff52943d409
https://hg.mozilla.org/mozilla-central/rev/eb129725f339
https://hg.mozilla.org/mozilla-central/rev/bb80569b28ad
https://hg.mozilla.org/mozilla-central/rev/6156f27bf791
https://hg.mozilla.org/mozilla-central/rev/44c5c9f4e34f
https://hg.mozilla.org/mozilla-central/rev/2db5ac710102
https://hg.mozilla.org/mozilla-central/rev/4b38958eb9d2
https://hg.mozilla.org/mozilla-central/rev/cd119f986edf
https://hg.mozilla.org/mozilla-central/rev/86c387214610
https://hg.mozilla.org/mozilla-central/rev/80524ce4789c
https://hg.mozilla.org/mozilla-central/rev/24b78368c9d4
https://hg.mozilla.org/mozilla-central/rev/9a88d7f1d951
https://hg.mozilla.org/mozilla-central/rev/859e1832aff0
Status: ASSIGNED → RESOLVED
Last Resolved: 8 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Iteration: --- → 57.1 - Aug 15
Depends on: 1389621
some talos improvements from this set of pushes:
== Change summary for alert #8695 (as of August 09 2017 22:23 UTC) ==

Improvements:

 13%  sessionrestore linux64 opt e10s     652.00 -> 567.25
 12%  sessionrestore_no_auto_restore linux64 opt e10s747.42 -> 661.25
  7%  sessionrestore linux64 pgo e10s     524.42 -> 489.92
  4%  ts_paint linux64 pgo e10s           856.29 -> 823.58
  3%  ts_paint linux64 opt e10s           971.54 -> 938.67
  3%  ts_paint_webext linux64 opt e10s    1,150.25 -> 1,115.75
  3%  sessionrestore windows7-32 opt e10s 775.54 -> 755.25
  2%  sessionrestore_many_windows linux64 opt e10s2,023.21 -> 1,980.08

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8695
(Assignee)

Comment 105

4 days ago
\o/
Woah, *clap* *clap*
Depends on: 1391117
(Assignee)

Updated

a day ago
Duplicate of this bug: 1300126
You need to log in before you can comment on or make changes to this bug.